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(core): Use globalThis for code injection #610

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Sep 23, 2024

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.

@lforst
Copy link
Member

lforst commented Sep 23, 2024

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)

@timfish
Copy link
Collaborator Author

timfish commented Sep 23, 2024

Yep, understood.

I guess this applies to the cli too which uses the same injected code?

@lforst
Copy link
Member

lforst commented Sep 23, 2024

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.

@timfish
Copy link
Collaborator Author

timfish commented Sep 23, 2024

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?

@lforst
Copy link
Member

lforst commented Sep 23, 2024

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.

@timfish
Copy link
Collaborator Author

timfish commented Sep 23, 2024

Perfect, it looks like the integration tests are all passing with the supported Node versions.

@timfish timfish marked this pull request as ready for review September 23, 2024 13:42
@timfish timfish self-assigned this Sep 26, 2024
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.

2 participants