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.

Sign up for GitHub

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

Jump to bottom

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

Open
domq opened this issue May 11, 2023 · 1 comment · May be fixed by #562
Open

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

domq opened this issue May 11, 2023 · 1 comment · May be fixed by #562
Labels
bug Something isn't working

Comments

@domq
Copy link

domq commented May 11, 2023

Fiddle at https://jsfiddle.net/2f6uzmb5/6/

Steps to reproduce:

  1. Import Goober “the old-school way” e.g. with 
    <script src="https://cdn.jsdelivr.net/npm/goober@2.1.13/dist/goober.umd.js" />
  2. Set window.target to something that internal function getSheet() won't like
  3. Call css()

Expected result: css() should return a fresh CSS class name.

Actual result:

TypeError: e.querySelector is not a function

(where e is presumably the minified variable that holds window.target inside getSheet())

From reading the relevant source code, it looks like setting any of window.p, window.g, window.o or window.k could also result in funny behavior.

This doesn't happen in a modern (ESM) environment, as they dropped the idea of squeezing the global object into this-less function calls.
jluxenberg added a commit to tryfoobar/goober that referenced this issue Sep 6, 2023
@jluxenberg
fix: use strict mode in function which use this
b6159d8 
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, "use strict" ensures that `this` will only have a value if it
is explicitly bound, and otherwise will be `undefined`.

Closes cristianbote#545
jluxenberg added a commit to tryfoobar/goober that referenced this issue Sep 6, 2023
@jluxenberg
fix: don't use globalThis when using this
7a26f07 
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
@jluxenberg jluxenberg linked a pull request Sep 6, 2023 that will close this issue
Open
@jluxenberg
Copy link

This will also happen when using esbuild to bundle, since it doesn't emit the "use strict" directive at the top of each bundled module.

See this esbuild-based repro: 2023-09-05_esbuild-x-goober_bug.tgz

and my fix: #562

@cristianbote cristianbote added the bug Something isn't working label Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: don't use globalThis when using this tryfoobar/goober
3 participants
@jluxenberg @domq @cristianbote