chore(shared): Browser global accessors [SDK-293] #2041
Conversation
|
| */ | ||
| export const browser = inBrowser() | ||
| ? rawBrowserGlobals | ||
| : new Proxy(rawBrowserGlobals, { |
There was a problem hiding this comment.
@tmilewski Could you please share some context here? Have we set up any specific test environments / eslint config to catch issues like these without testing with an actual expo app?
Unless I'm missing something, trying to access these in non-browser environments would result in errors even without the Proxy here.
| @@ -52,8 +78,8 @@ export function userAgentIsRobot(userAgent: string): boolean { | |||
| * @returns {boolean} | |||
| */ | |||
| export function isValidBrowser(): boolean { | |||
| const navigator = window?.navigator; | |||
| if (!inBrowser() || !navigator) { | |||
| const navigator = browser.window.navigator; | |||
There was a problem hiding this comment.
This changes the behavior - instead of returning false we're now throwing. Is this expected?
| if (!inBrowser() || !navigator) { | ||
| const navigator = browser.window.navigator; | ||
|
|
||
| if (!navigator) { |
There was a problem hiding this comment.
Ditto.
This refactor for these two cases might be fine, however I don't think we can simply replace the usage of the inBrowser() helper with the new proxy-based approach as the two are not equivalent.
There was a problem hiding this comment.
Thinking about this a bit more, if this piece of code runs in an expo app, instead of returning false, its going to throw and stop app execution upon accessing browser.window
|
It will be great as part of this task to add some linter or build rules for Expo so that building fails if any of these are used in Expo during development. |
|
Closing in favor of implementing linting rules. |
Description
Create an accessor for browser globals such that they fail on non-browser environments.
I have follow-up work, converting call-sites to this new process, but I ultimately think we should update as-needed and in-line with other work to avoid potentially missed issues per cached modules in Jest.
SDK-293
Checklist
npm testruns as expected.npm run buildruns as expected.Type of change
Packages affected
@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/clerk-expo@clerk/fastifygatsby-plugin-clerk@clerk/localizations@clerk/nextjs@clerk/clerk-react@clerk/remix@clerk/clerk-sdk-node@clerk/shared@clerk/themes@clerk/typesbuild/tooling/chore