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

remove window variable #13367

Closed
ry opened this issue Jan 13, 2022 · 11 comments
Closed

remove window variable #13367

ry opened this issue Jan 13, 2022 · 11 comments
Labels
suggestion suggestions for new features (yet to be agreed)

Comments

@ry
Copy link
Member

ry commented Jan 13, 2022

The background for this is that many existing web frameworks incorrectly use window as a feature detect for web browsers. This breaks them when server side rendering in Deno, as they think they are running in a browser.

The web frameworks should be feature detecting document if they want to know if they have access to the DOM, not window.

Nonetheless this practice is so prevalent in the ecosystem, that it would be difficult to change all of these wrong feature detects. This Sourcegraph query illustrates how widespread this is: https://sourcegraph.com/search?q=context:global+typeof+window&patternType=literal.

Because of this, we should consider removing the window global from Deno at the earliest possible date (likely Deno 2.0).

@ry ry added maybe 2.0 feat new feature (which has been agreed to/accepted) labels Jan 13, 2022
@andreubotella
Copy link
Contributor

andreubotella commented Jan 14, 2022

The WPT tests require window to be in the global, so if it's removed from Deno, it would have to be re-added either in the WPT bundle, or in bootstrapMainRuntime if the --enable-testing-features-do-not-use flag is passed.

@lucacasonato lucacasonato added suggestion suggestions for new features (yet to be agreed) and removed feat new feature (which has been agreed to/accepted) labels Jan 14, 2022
@ry
Copy link
Member Author

ry commented Jan 14, 2022

@andreubotella can you give an example? I was trying to grep through wpt to see how difficult this would be but couldn’t find any usages that would block our tests.

@andreubotella
Copy link
Contributor

It's true that most WPT tests that we run, even the *.window.js ones, don't depend on the presence of window, but there are some that do. For example, the webstorage tests use the window[name] pattern all the time, where name can be either "localStorage" or "sessionStorage" (https://github.com/web-platform-tests/wpt/blob/master/webstorage/defineProperty.window.js). But it seems like they're few enough that they could be fixed upstream.

@heypiotr
Copy link

Would it also make sense to remove the navigator object?

Arguably, using navigator during SSR is definitely incorrect, but I noticed that some libraries define top-level constants a'la

const IS_CHROME = typeof navigator !== "undefined" && navigator.userAgent.includes("Chrome")

... and that crashes in Deno on import.

@lucacasonato
Copy link
Member

That should be fixed by actually specifying a navigator.userAgent (see #14362).

@Industrial
Copy link
Contributor

Hi! Recently I sought to use React Testing Library to test my components in my Fresh project but was unable to get it to work (denoland/fresh#427). Part of the problem is that the window object is read only and I cannot assign a document to it (provided by e.g. JSDOM or deno-dom). I believe removing the window variable from Deno and supplying it from one of hte DOM libraries (https://deno.land/[email protected]/jsx_dom) is the way to go.

@matthew-dean
Copy link

The web frameworks should be feature detecting document if they want to know if they have access to the DOM, not window.

I'm not sure I agree with this statement. A window indicates a window. A visual window i.e. a browser or browser-like environment.

@tmcw
Copy link

tmcw commented Feb 6, 2023

Just a datapoint from in an application - trying to get an existing application ported to remix.run and Deno has been difficult in part because, yeah - I'm seeing libraries that detect window in lots of places, which is causing them to then assume that, say, document.queryElement exists. Removing window or making it removable would be a big help.

@andreubotella
Copy link
Contributor

window is already removable by doing delete globalThis.window.

@dsherret
Copy link
Member

dsherret commented Feb 7, 2023

Once this is removed, for backwards compatibility we can recommend people do globalThis.window = globalThis;.

Actually, would need to be:

import "data:text/javascript,globalThis.window = globalThis;";

FilipChalupa added a commit to FilipChalupa/collapsible-react-component that referenced this issue May 24, 2023
bartlomieju added a commit that referenced this issue Jan 24, 2024
This commit deprecates `window` global and adds deprecation
notice on each use of `window`.

We decided to proceed with removal of `window` global variable in Deno
2.0. There's a lot of code
in the wild that uses pattern like this:
```
if (typeof window !== "undefined) {
  ...
}
```
to check if the code is being run in browser. However, this check passes
fine in Deno and
most often libraries that do this check try to access some browser API
that is not available
in Deno, or use DOM APIs (which are also not available in Deno).

This situation has occurred multiple times already
and it's unfeasible to expect the whole ecosystem to migrate to new
check (and even if that
happened there's a ton of code that's already shipped and won't change).

The migration is straightfoward - replace all usages of `window` with
`globalThis` or `self`.
When Deno encounters use of `window` global it will now issue a warning,
steering users
towards required changes:

```
Warning
├ Use of deprecated "window" API.
│
├ This API will be removed in Deno 2.0. Make sure to upgrade to a stable API before then.
│
├ Suggestion: Use `globalThis` or `self` instead.
│
├ Suggestion: You can provide `window` in the current scope with: `const window = globalThis`.
│
└ Stack trace:
  └─ at file:///Users/ib/dev/deno/foo.js:7:1
```

Ref #13367.
@bartlomieju
Copy link
Member

As of #25213 this is now in effect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

No branches or pull requests

9 participants