-
Notifications
You must be signed in to change notification settings - Fork 903
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(labs/ssr): Prevent exposure of window object on globalThis #3431
Conversation
…globalThis. Fixes #3412
🦋 Changeset detectedLatest commit: d409602 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Thank you for the PR! Were you able to get the CLA sorted out? As noted below, we'd like to simply make no window
the de facto behavior with no need for the option.
Could you also do a npm run changeset
to note this change? It would be a breaking change so a major bump would make sense. I'd be happy to handle that part too if you'd like.
Still working on the CLA. I will make the other changes as requested |
PR updated with the new functionality and a changeset. Interestingly, the server and the render-lit tests both required |
I'm still working on the CLA - legal are reviewing it now as there's other people in the org affected too |
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.
@steveworkman Any update on the CLA? If it's looking like more trouble, I can make a separate PR referencing this one for credit if you don't mind. Really appreciate it!
CLA signed, awaiting return from Google 🤞 |
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.
One last bit of cleanup, #3367 just got merged so bringing in the latest from main branch will remove the need for window
references in the VM module tests.
That'll also fix the breaking tests in the eleventy-plugin-lit
package's test.
Co-authored-by: Alexander Marks <[email protected]>
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.
Thank you! We're queuing up some breaking changes to SSR for the next release in a separate branch.
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.
Thanks for this PR!
Nope, all set! Looking at a bit of coordination for the monorepo but I'll work on pressing the merge. |
Fixes #3412
I made an extension to the existing
installWindowOnGlobal
method which defaults to installingwindow
. This makes this change non-breaking and you can opt-in to the functionality.I've tested this with the nuxt3 plugin and it's working well - component authors will need to be exceptionally careful about using
window
, perhaps that analysis can be added to the testing library