deno icon indicating copy to clipboard operation
deno copied to clipboard

remove window variable

Open ry opened this issue 2 years ago • 10 comments

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 avatar Jan 13 '22 21:01 ry

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.

andreubotella avatar Jan 14 '22 15:01 andreubotella

@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.

ry avatar Jan 14 '22 18:01 ry

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.

andreubotella avatar Jan 17 '22 06:01 andreubotella

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.

heypiotr avatar Apr 22 '22 12:04 heypiotr

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

lucacasonato avatar Apr 22 '22 12:04 lucacasonato

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 (https://github.com/denoland/fresh/issues/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.

Industrial avatar Oct 14 '22 20:10 Industrial

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.

matthew-dean avatar Feb 03 '23 18:02 matthew-dean

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.

tmcw avatar Feb 06 '23 23:02 tmcw

window is already removable by doing delete globalThis.window.

andreubotella avatar Feb 06 '23 23:02 andreubotella

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;";

dsherret avatar Feb 07 '23 14:02 dsherret