-
-
Notifications
You must be signed in to change notification settings - Fork 2
Refactor: script injectors #39
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: main
Are you sure you want to change the base?
Conversation
| // @todo find proper solution to ensure page view is tracked after tracker api is initialized | ||
| window.setTimeout(() => { | ||
| trackPageView(location) | ||
| redBoxLtdTracker && redBoxLtdTracker.push(['page', location.pathname]) |
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.
relevant
| </svg> | ||
| ) | ||
|
|
||
| const ScriptInjector: React.FC = () => { |
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.
this example injects a tracking pixel
|
|
||
| // ensure that the tracker script will be initialized once in runtime | ||
| let wasInitialized = false | ||
| const ScriptInjector: React.FC = () => { |
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.
this integration injects a script tag and manipulates window as many tracking scripts do
| interface RedBoxLtdTracker extends Array<Array<String>>, Tracker {} | ||
|
|
||
| // For usage non-react context when tracking page views (Docusaurus, Gatsby, ...) | ||
| // @todo we need to save-guard this by checking if integration is actually enabled |
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.
@todo 🤔
| } | ||
| React.useEffect(() => { | ||
| if (isEnabled && !tracker) { | ||
| locateTracker('rbltd', setTracker) |
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.
relevant, this ensures react can track to the correct object as soon its available
| "@types/react": "^16.9.11", | ||
| "@types/react-dom": "^16.8.4", | ||
| "parcel": "1.12.3", | ||
| "parcel-plugin-static-files-copy": "^2.6.0", |
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.
relevant, copies our red-box-ltd.js and pixel.png into the example public dir
| Boolean(i.ScriptInjector) | ||
| ) | ||
| .map(({ ScriptInjector, id }) => <ScriptInjector key={id} />) | ||
| }, [config, decisions, isMounted]) |
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.
relevant, this is the bug fix for multiple wrapper/injector components
| export type Status = 'idle' | 'loading' | 'ready' | 'error' | ||
| export type ScriptElt = HTMLScriptElement | null | ||
|
|
||
| export function useScript( |
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.
relevant, injects the script tag and removes it when dismounted
| return status | ||
| } | ||
|
|
||
| export function locateTracker( |
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.
relevant, finds our tracker from the external script in the window object, to ensure user userTracker() hook returns the proper tracker
| <tr> | ||
| <td>WrapperComponent</td> | ||
| <td>{WrapperComponent ? '✅' : '⛔️'}</td> | ||
| <td>Injects <script/> tag</td> |
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.
This makes now more sense :)
| @@ -0,0 +1,33 @@ | |||
| module.exports = { | |||
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.
relevant, our first eslint rule.
This ensures people don't create integrations using document.createElement('script') to inject script tags. They should use our helper.
2f23060 to
0fb0cf0
Compare
There is no need to wrap the whole application with our integration components, thats why we applied #33
This PR refactors the code so it reflects what these integrations actually do:
It also fixes a bug that prevents multiple WrapperComponents to render.