-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Separate logic native detector #3732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Conversation
Currently the new Detector is named |
export type { NativeDetectorProps } from './v3/Detectors/common'; | ||
export { NativeDetector } from './v3/Detectors/NativeDetector'; | ||
|
||
export { LogicDetector } from './v3/Detectors/LogicDetector/LogicDetector'; | ||
export { DelegateDetector } from './v3/Detectors/LogicDetector/DelegateDetector'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe let's also create index.ts
in v3/detectors
(and yes, I'd use lowercase for that directory 😅) which will export these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -14,7 +14,9 @@ export const DetectorContext = createContext<DetectorContextType | null>(null); | |||
export function useDetectorContext() { | |||
const ctx = useContext(DetectorContext); | |||
if (!ctx) { | |||
throw new Error('Logic detector must be a descendant of a Native Detector'); | |||
throw new Error( | |||
'Logic detector must be a descendant of a delegate detector' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Logic detector must be a descendant of a delegate detector' | |
tagMessage('Logic detector must be a descendant of a delegate detector') |
Description
I did some performance tests on the
NativeDetector
after adding changes that handle attachingLogicDetector
, which revealed that the new logic adds quite a bit of overhead, but only on the JS side. New logic on the native side does not seem to have a significant effect.We concluded that the best solution is to create a separate component that will have all functionalities of
NativeDetector
and also allowLogicDetector
attachment, while NativeDetector will be reverted to how it had been before implementingLogicDetector
.Test plan
This example, which has
DelegateDetector
aboveLogicDetector
should workAnd this one which uses
NativeDetector
aboveLogicDetector
should throwLogic detector must be a descendant of a delegate detector