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

fix: don't use globalThis when using this #562

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jluxenberg
Copy link

Without "use strict", this will be set to globalThis which is typically the window object. If certain properties are set on the window object (e.g. "window.target"), this can cause problems for the css and styled functions.

Instead, check that this has been set to something other than globalThis before using it. This ensures that this will only have a value if it is explicitly bound, and otherwise will be undefined.

Note: I tried to add "use strict"; directives where needed, but couldn't get them to "survive" the build process -- they were removed. This fix seems to work just fine.

Closes #545

Without "use strict", `this` will be set to `globalThis` which is
typically the window object. If certain properties are set on the
window object (e.g. "window.target"), this can cause problems for the
`css` and `styled` functions.

Instead, check that `this` has been set to something other than
`globalThis` before using it. This ensures that `this` will only have
a value if it is explicitly bound, and otherwise will be `undefined`.

Note: I tried to add "use strict"; directives where needed, but couldn't
get them to "survive" the build process -- they were removed. This fix
seems to work just fine.

Closes cristianbote#545
@vercel
Copy link

vercel bot commented Sep 6, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
goober-rocks ❌ Failed (Inspect) Sep 6, 2023 5:45am

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 6, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7a26f07:

Sandbox Source
Vanilla Configuration

jluxenberg added a commit to jluxenberg/react-hot-toast that referenced this pull request Sep 6, 2023
This fork of Goober has this PR merged:
cristianbote/goober#562

which fixes an issue caused by sites that set "window.target" to some
non-undefined value.
@cristianbote
Copy link
Owner

Heya @jluxenberg,

Thank you so much for this PR and for the details. goober uses microbundle to bundle it up, microbundle has a --strict option. Have you tried passing that? Meaning here a --strict flag should be passed https://github.com/cristianbote/goober/blob/master/package.json#L76C72-L76C72

@jluxenberg
Copy link
Author

Heya @jluxenberg,

Thank you so much for this PR and for the details. goober uses microbundle to bundle it up, microbundle has a --strict option. Have you tried passing that? Meaning here a --strict flag should be passed https://github.com/cristianbote/goober/blob/master/package.json#L76C72-L76C72

I did try that (and added in all the places where microbundle is invoked) but it didn't seem to change the build artifacts at all.

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.

css() taken by surprise when this is window (in “old-school” JS environment)
2 participants