-
Notifications
You must be signed in to change notification settings - Fork 36
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(core): Use globalThis
for code injection
#610
base: main
Are you sure you want to change the base?
Conversation
Generally, I like the change because X times 90 bytes is a lot, but I think we should probably do this in a major 🤔 Technically, people could be using the bundler plugins for applications targeting older versions. We don't really have a version policy for these packages. (maybe we should) |
Yep, understood. I guess this applies to the cli too which uses the same injected code? |
Correct! I am trying to think of reasons why we shouldn't cut a major but I can't really come up with something. All the downstream consumers of this package (our SDKs) already define a compatible browser range with this change. |
I haven't updated the e2e snapshots and couldn't get them to run locally so guess they'll fail if they had run. If I push a branch on this repo rather than my own fork they should run? |
I think we deactivated the e2e tests a long time ago. Feel free to ignore them. The way to go is integration tests. I think I am gonna remove the e2e tests. |
Perfect, it looks like the integration tests are all passing with the supported Node versions. |
Since all our supported platforms now support
globalThis
, from v8 of the JavaScript SDK we now use this to read globals and I recently changed this for the nextjs SDK code injection. We can also do this for the bundler plugins since this saves ~90 bytes from each injected code snippet.