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

Regression in global property lookup #202

Closed
sholladay opened this issue Nov 11, 2019 · 0 comments · Fixed by #203
Closed

Regression in global property lookup #202

sholladay opened this issue Nov 11, 2019 · 0 comments · Fixed by #203
Labels
bug Something isn't working

Comments

@sholladay
Copy link
Collaborator

sholladay commented Nov 11, 2019

PR #153 was intended to make it easier to mock globals that Ky relies on.

But it actually broke my app's mocking...

In fact, in environments that have globalThis (including Node.js), the logic for determining which global object to use is completely broken. It will always choose globalThis no matter what. Anyone using an artificial window or self (e.g. with jsdom), which is necessary in React projects, is no longer able to put things there and have Ky pick up on them. They just get ignored.

This is happening because the return statements were removed from here. Instead, the if statements now "fall through" here until it hits globalThis, which doesn't have the same properties as window in my case, yet Ky chooses to use globalThis anyway, even though the property doesn't exist there.

Another consequence of falling through like this, even if your environment doesn't have globalThis, is that the order of precedence is reversed from what it was previously. Before it would stop at the earliest match found, now it stops at the last match found.

I would prefer to revert this commit and rethink how it works, but a simple temporary fix for the first problem would be to check that the property exists on globalThis before setting the parent to globalThis.

// cc @luxferresum

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant