Skip to content
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

feat(utils): Use consts rather than getGlobalObject function #5821

Closed

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Sep 26, 2022

As discussed here, this PR:

  • Replaces getGlobalObject with a const GLOBAL_OBJ which retains the runtime detection from feat(utils): Modern implementation of getGlobalObject #5809
  • Also includes a WINDOW const which returns GLOBAL_OBJ but with typeof GLOBAL_OBJ & Window type.
    • This will eventually move to @sentry/browser to ensure stricter typing around the global object
    • Maybe this should be called WINDOW_GLOBAL to remove any ambiguity?

It's worth noting that @sentry/node and downstream node code doesn't seem to make use of getGlobalObject and is already using global and I'm not sure if anything is gained by modifying these.

@timfish timfish changed the title feat(utils): Use consts rather than getGlobalObject feat(utils): Use consts rather than getGlobalObject function Sep 26, 2022
@AbhiPrasad
Copy link
Member

AbhiPrasad commented Sep 26, 2022

hmm I wish we could turn on bundle size bot for you - does this give any savings there?

Also, when this gets to a good place, could we split this up so we change each package one at a time?

@timfish
Copy link
Collaborator Author

timfish commented Sep 26, 2022

One thing that makes this PR a little bit risky is the fact that the TypeScript compilation doesn't seem to complain about the node.js global appearing anywhere. This means there are no warnings if an undefined variable appears anywhere in code named global!

@timfish
Copy link
Collaborator Author

timfish commented Sep 26, 2022

Also, when this gets to a good place, could we split this up so we change each package one at a time?

Yeah, I'll split it up because this PR has become huge!

@timfish
Copy link
Collaborator Author

timfish commented Sep 26, 2022

So bundle.min.js is 83 bytes smaller vs before the changes to getGlobalObject in #5809 🤷‍♂️

@timfish timfish marked this pull request as ready for review September 26, 2022 21:32
@lobsterkatie
Copy link
Member

One thing that makes this PR a little bit risky is the fact that the TypeScript compilation doesn't seem to complain about the node.js global appearing anywhere. This means there are no warnings if an undefined variable appears anywhere in code named global!

This feels fishy to me. It should be complaining... Is there a place where you see it in a package which doesn't have node types?

@timfish
Copy link
Collaborator Author

timfish commented Sep 27, 2022

Is there a place where you see it in a package which doesn't have node types?

Just double checked and I can stick global anywhere in utils and there is no compilation error.

I suspect it's because @types/node is in the root node_modules and TypeScript just blindly loads everything in @types/* while following node module resolution.

@timfish
Copy link
Collaborator Author

timfish commented Oct 5, 2022

Closing this in favour of multiple smaller PRs

@timfish timfish closed this Oct 5, 2022
@timfish timfish deleted the feat/remove-getGlobalObject branch October 17, 2022 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants