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

window.Promise broken: Feedback on window being removed. #25797

Closed
josephrocca opened this issue Sep 22, 2024 · 16 comments
Closed

window.Promise broken: Feedback on window being removed. #25797

josephrocca opened this issue Sep 22, 2024 · 16 comments

Comments

@josephrocca
Copy link
Contributor

josephrocca commented Sep 22, 2024

Please excuse the dramatic title. It sounds like the ship has sailed on this, and if it has then I'll just sigh and be a little less enthusiastic about the direction of the project, but the announcement did solicit feedback, so I'm hoping there's a chance this can be changed. Here's my feedback:

IIRC (correct me if I'm wrong) there was a clear/explicit 'promise' by @ry in the early days of Deno that web compatibility would never be broken. It may have been on Github, but the closest I could find was this:

The browser standards define these interfaces, not us. Any corrections issued by us are bug fixes, not interface changes.

I understand that trade-offs need to be made. But I'm not lawyering or nitpicking wording when, in referencing the above quote, I say that web compatibility is in large part what defined Deno as a project. And not just the web compatibility, but the degree of backwards compatibility that the web has. This is what sets it apart and makes it exciting. A "timeless runtime", like the web, that can run any JavaScript that could reasonably be expected to run in a server environment. Not NodeScript or DenoScript, or BunScript, but JavaScript. What an exciting vision!

This change breaks the original promise/vision in about as big a way as you possibly could (bolded for tl;dr of this whole post, not shouting). There is a lot of JavaScript in the world that will plainly no longer work at all in Deno after this change. Stuff like this:

  • "TextEncoder" in window
  • window.atob
  • window.navigator

Code like this is everywhere on StackOverflow and Github. Ask Claude for some web-compatible code and it's not uncommon to get code with window in it. It all breaks with the proposed change. Deno web compat, in broad terms, is basically downgraded to the Node.js level of "except for <arbitrary thing for no clear reason (to a newcomer) and with no fix intended> Deno is web compatible". I'm fine with "but X doesn't make sense in a server environment", but not "we removed and very core/central thing, which breaks compatibility with the web, and we did it because of a Node.js environment detection edge case".

But more than that, the "vision" kinda feels dead with this change. It sounds dramatic, but this change is really big - the promise is unequivocally broken. This is not the "web" runtime anymore - I don't decide on the rules here, I just know that breaking huge amount of normal/valid JavaScript code means that a runtime can't wear that hat.

Please, (I'm down on my knees) find a (even very hacky) way to solve this which doesn't break web compatibility. E.g. enabling this change in npm: only, or something. Please be ambitious in thinking about other ways to solve the problem that this change is intended to solve. E.g. manually tag NPM modules. Or detect and rewrite common runtime check code (even if it only covers 95% of cases). Anything but this. Please try really hard to move the burden to the people with rare edge cases, rather than burdening the common cases, and burning down the explicitly-stated "vision" of the project for said edge case 🙏

@josephrocca josephrocca changed the title Promise broken: Feedback on window being removed. Promise broken: Feedback on window being removed. Sep 22, 2024
@josephrocca josephrocca changed the title Promise broken: Feedback on window being removed. window.Promise broken: Feedback on window being removed. Sep 22, 2024
@bartlomieju
Copy link
Member

"we removed and very core/central thing, which breaks compatibility with the web, and we did it because of a Node.js environment detection edge case".

This is actually a bit backwards - the reason we removed window variable is because there are lot of libraries that do typeof window !== "undefined" to detect if they are running in the browser an expect that the DOM API will be present (ie. document global). This check is obviously wrong, as it should probe for existence of document global, but it is what it is. This had nothing to do with Node.js environment detection edge case.

But more than that, the "vision" kinda feels dead with this change. It sounds dramatic, but this change is really big - the promise is unequivocally broken. This is not the "web" runtime anymore - I don't decide on the rules here, I just know that breaking huge amount of normal/valid JavaScript code means that a runtime can't wear that hat.

Deno is still Web compatible as it possibly can, but window global just didn't work out correctly in the end, leading to more pain and burden because of broken libraries that work perfectly fine if it wasn't for the window global. It's a global variable that was a mistake to be added in the first place, the remaining hundreds Web APIs still work as they worked before and they will continue to work.

Please, (I'm down on my knees) find a (even very hacky) way to solve this which doesn't break web compatibility. E.g. enabling this change in npm: only, or something.

This is not really possible, because the problem with window global is present in any dependency - remote or local; a lot of libraries served from CDNs don't work correctly because they "think" they are executed in the browser tab with DOM available.

@Hajime-san
Copy link
Contributor

Hajime-san commented Sep 22, 2024

As written in this issue, I don't think this is a problem with browser compatibility.

It's simply a case of the user's code using window incorrectly.
And while Deno places great importance on browser compatibility, at the same time, Deno has a responsibility not to behave as a browser. I think the decision to remove window is made precisely to avoid breaking the behavior and compatibility of existing code.
On the other hand, there will be occasions when you write code that relies on window.
In that case, if you add the line globalThis.window = globalThis;, I think that for global APIs supported by Deno, it will basically behave the same as a browser.

A bit off topic, but Deno recently gained the ability to use Node.js global process independently of the Node.js compatibility mode.

This means that Deno is no longer a runtime that completely hides Node.js.
However, process is still there to make Node.js-compatible code work well, and deno lint has added a rule to encourage the use of node:process instead of process.

It's programmer nature to want to strive for perfection and flawlessness.
The Deno team's ideals are still held high, and I salute the careful work they're doing on this.

@josephrocca
Copy link
Contributor Author

josephrocca commented Sep 22, 2024

libraries that do typeof window !== "undefined" to detect if they are running in the browser [...] This had nothing to do with Node.js environment detection edge case.

What are they trying to detect, if not whether they are running in a browser environment vs a Node.js environment? This seems like it has everything to do with runtime detection. I think this particular point might just be a misunderstanding due to my poor phrasing or something.

a lot of libraries served from CDNs don't work correctly because they "think" they are executed in the browser tab with DOM available

I am really interested in how common this is, and what DOM-related things they try to access. I use unpkg, esm.sh, jsdelivr+esm with Deno, and haven't run into this problem (but I'm only one data point, of course).

It seems a bit strange for a library to work in Node.js and the browser, to have some very critical DOM related functionality. What aspect of the DOM are they trying to interact with? And if there are e.g. 500 common packages that cover 99% of problems, then why not just tag their (immutable) CDN URLs and do a simple source patch/conversion?

Are you 95%+ sure that you're not making a bad trade-off here? I.e. a trade-off that serves a (temporary) edge case at the expense of something which is fundamentally more valuable to the Deno project? I'm wondering what your confidence level is, roughly, on this. Obviously if you're 51% sure, then that's fine too, but I'm just wondering "how crazy I am", given that this seems very clearly like a bad decision to me.

@Hajime-san "if you add the line globalThis.window = globalThis;, I think that for global APIs supported by Deno, it will basically behave the same as a browser."

This is the wrong way around imo. Deno is the web-compatible server runtime. That was almost the whole point. If are some rare edge cases, then the burden should be on those rare edge cases to add the workarounds and fixes, rather than sacrificing a very core part of Deno's philosophy. We're talking about window here! It's everywhere in web-compatible JavaScript. If I import some code that has if(window.AbortSignal) { ... } and that crashes, then this is not "the web-compatible runtime" anymore, and beyond the "how is this different from Node.js" aspect, it just makes Deno annoying to use, in the same way that I was annoyed by Node randomly being web-incompatible in various ways, for seemingly no reason.

It's programmer nature to want to strive for perfection and flawlessness.

I agree, but again I think you've got it the wrong way around: I think removing window is striving too much for perfection. I think the team should make a very hacky solution here to fix this. The reluctance for hackiness is, I think, causing the team to pick a bad solution to the problem.

  1. Fix both problems, but use hacky/gross solution
  2. Fix one problem, by sacrificing something important, but it's cleaner

The team is choosing 2 here. I'm advocating for 1.

The Deno team's ideals are still held high, and I salute the careful work they're doing on this.

I also salute them, whichever path they take here. It's an incredible project! It basically kickstarted a new era of server-side JS.

bartlomieju added a commit that referenced this issue Sep 22, 2024
This commit adds better handling for terminal errors when
`window` global is used. This global is removed in Deno 2,
and while we have lints to help with that, an information and
hints are helpful to guide users to working code.

Ref #25797
@Hajime-san
Copy link
Contributor

this is not "the web-compatible runtime" anymore, and beyond the "how is this different from Node.js"

Yes, this is the truth.

It's all a matter of how you interpret the word "web-incompatible" but I think Deno is merely a subset of the web, and never aimed to be a complete set.
For example, window.scrollbars crashes in Deno.
Since Deno is not a browser after all, it is up to Deno itself to decide what to support. I think it just so happens that it was a window that appears frequently.

As for the solution, if you want to go "web-incompatible" while still implementing window, there is no hack, and Deno probably need to implement all APIs under window.
This may be possible around Deno v10, and bootstrap performance will worsen as the number of globally defined APIs increases.

@lucacasonato
Copy link
Member

What are they trying to detect, if not whether they are running in a browser environment vs a Node.js environment? This seems like it has everything to do with runtime detection. I think this particular point might just be a misunderstanding due to my poor phrasing or something.

They are trying to detect whether they are in a browser vs in a SSR environment. A nice example is use-swr: https://github.com/vercel/swr/pull/754/files. It was using window presence to mean "has dom, is browser, should treat as the place where client side code executes" (which is wrong, it should have used document presence), but it is what it is.

They added explicit code to check whether there is a Deno global, to work around this, but unfortunately many other projects did not. That meant that many frontend libraries that could run either on client or server considered deno a "client" not a "server".

I was of the opinion that technical purity (ie keep window) should win over pragmatism (remove window) in this regard for a very long time. But it turned out that essentially many major adopters of Deno did delete globalThis.window before loading code because window was more harmful than good. That means we picked the wrong default for the current state of the world. Maybe Deno 3 or Deno 4 can re-add the window global, if the world is ready for that, but very unfortunately it is not ready yet.

@marvinhagemeister
Copy link
Contributor

marvinhagemeister commented Sep 23, 2024

FYI: The reason frontend code uses typeof window === "undefined" instead of typeof document === "undefined" is that the document object typically exists within test environments. It's common to load something like jsdom, happy-dom or something else in the test environment to provide the DOM interface. The most popular testing library used with React @testing-library/react does exactly this. This makes testing frontend components easier as most tests are typically in the spirit of "does this text become visible in the HTML when I pass in this prop".

@josephrocca
Copy link
Contributor Author

josephrocca commented Sep 23, 2024

@Hajime-san: For example, window.scrollbars crashes in Deno.

It doesn't crash in Deno v1, to be clear - it just returns undefined, and very few people would try to run scrollbar-related code on a server.

@lucacasonato: They added explicit code to check whether there is a Deno global, to work around this, but unfortunately many other projects did not.

So, correct me if I'm wrong here, but the choices here were roughly:

  1. Remove window, breaking a substantial portion of valid JavaScript that exists in the world, including code previously written for Deno, and downgrading the soul of Deno ("~any valid JS that could be reasonably expected to work in a server environment will 'just work', and is as backward-compatible as the web") to something like "It's a like Node.js or Bun, but with permissions and [as of writing] is a few steps ahead on web compat".
  2. Submit a pull requests to popular SSR libs, and patch the old versions. (People trust the Deno binary to do this sort of thing, btw. If they didn't, why on earth are they executing it on their machine.)

I say "a few" pull requests because the concern here seems to be "major adopters", using non-obscure libraries/frameworks. And once the ecosystem catches on, this issue will go away, so there's no perpetual labor required here. I don't use SSR libs, but I've never had a problem with this. If patching the top 10 frameworks wouldn't mostly solve this issue, then do you have any sense for how prevalent this problem is (beyond popular frameworks that could be easily patched)?

I don't understand how the decision ended up being remove window - of all the possible choices to solve this. Does the team think this is the "proper" solution? Or is it more like "we need to keep this company alive, unfortunately don't have time to do this properly - i.e. it's kinda existential". I can understand the latter, of course. But a lazy solution to this that is maybe 80% as good as the 'patching' approach would be just adding warning letting them know they might need to add --no-window if common-and-troublesome module imports or code snippets are detected.

If "breaking a few percent of all valid JS code in existence" is the final fallback, then I think you should be willing to get extremely creative in maneuvering out of this. This is Deno we're talking about! This ain't no run-of-the-mill, code-won't-work-in-2-years runtime.

And, as an aside, I'm not averse to dying inside a little by adding globalThis.window=globalThis, but it doesn't necessarily work because now you've split the ecosystem by encouraging library maintainers to do the wrong thing. Import A is written for the web - nice, that means it'll work in Deno! Okay, so we need to add globalThis.window=globalThis, kinda gross, but no big deal. Oh, wait, turns out it's not compatible with import B because the standard practice has settled on checking window, which is what it does. So the globalThis.window=globalThis one-liner fails to help us in this case, resulting in a bit more dying inside.

I would be really surprised if the best solution to this whole predicament (in hindsight, and/or with omnipotence, or whatever) turned out to be "remove window, and maybe do another breaking change(!) by adding it back later idk".


An aside on backwards-compat:

deno run https://archive.org/file/846384673/game.js

Isn't this amazing? It's code from 2020 and it still runs perfectly! This is what sets Deno apart. They have a vision, and they take it seriously. It's timeless - like the web! A web browser for the command-line, in all the good senses.
-- Some person/robot on a separate timeline, circa 2050

I've always had "stable interfaces" as one of my "interests" in my twitter bio. And at parties I sometimes inappropriately segue into how surprisingly-naive platforms can be in underrating backwards-compatibility. I think the TL;DR is breaking a backwards-compat contract doesn't feel very costly when you do it - it's just a few small/annoying fixes, after all! But the cost from breaking this contract comes from what it implies about the future of project, in the minds of the devs who might consider choosing it over something else. The cost comes from how it changes "what the project is". I think this "contract" is a large part of what makes the web such a force to be reckoned with, when compared to other platforms.

I thought Deno was the server runtime that really saw the value in holding itself to a strong backwards-compat contract like this (and also realized how valuable it is to "hitch a ride on the juggernaut's back" by being as close to the web platform as it as is reasonable for a server-side runtime). I don't think I misinterpreted, and I don't think y'all miscommunicated. It's hard for me to understand how this trade-off calculation plays in Deno's favor in the long term, given that there are other ways to fix this.


@marvinhagemeister: The reason frontend code uses typeof window === "undefined" instead of typeof document === "undefined" is that the document object typically exists within test environments.

So if I understand correctly: Deno is breaking away from the core vision (due to an issue that's solvable via other approaches) for people who are running code which patches their environment to look like a browser, but also don't want their environment to be seen as a browser by other imports. A tragedy and a comedy all in one.

There's no need for this thread to continue for the sake of placating me, of course. I may become the Joker, but I'll survive. This issue can be closed if there's zero chance of any reconsideration here. If that's the case, thank you for hearing me out - I understand that this is a "judgement call" type of decision. If there is any interest in further discussion, I'm happy to keep discussing, of course.

@marvinhagemeister
Copy link
Contributor

Deno's main goal is to be used by developers. It tries to reuse and built upon as many web APIs as possible, but it is not a browser per se.

When looking at the window global we noticed that more code broke by having the global, compared to not having it. Code that targets Deno directly typically uses globalThis instead of window. There might be a library or two that does use window, but those were very few in numbers compared to the ones using globalThis.

The problem is that a significant portion of the existing JS ecosystem depends on the window global only being available inside browser environments. Telling 10 different projects to update their code is easier said than done, especially when it involves breaking their own test setup and the one that their users use.

That said, as you articulated, this decision is a tradeoff between being "pure" to Deno's original vision vs lowering the adoption barrier for a significant portion of the JS ecosystem. That portion is to big to ignore, and we haven't heard criticism about removing the window global so far. This issue here is the only one so far. Although I can't predict the future, it's unlikely that we're going to add back the window global.

@lucacasonato
Copy link
Member

lucacasonato commented Sep 23, 2024

@josephrocca Do you want to jump on a call with me to talk through this? I really want to explain our rationale to you more, but typing out individual responses to all of your feedback is not something I have time to do right now. You can reach me at <firstname>@deno.com :)

@josephrocca
Copy link
Contributor Author

josephrocca commented Sep 23, 2024

It's okay, but thank you. I think I understand all the core arguments. I wish there were any remotely viable way of working around this, but I'll trust that you've really thought this through, and this is basically impossible to do in any other way - even very hacky ways.

I'm a bit sad, of course, but there's still a lot to love about Deno outside of the strong back-compat and web-compat aspects of the original vision, and perhaps I'm much more sensitive on these points than the majority of Deno users, having come here quite early for that explicit reason. For how 'small' a change it is, it's a surprisingly big oof for me - I guess Deno was in a different category from Node.js and Bun in my mind. But if Deno is at risk of not getting enough adoption to flourish, and there are no ways other around this issue, then trade-offs like this can make sense of course.

I really appreciate your responsiveness and patience with me on this! 🫡

@Hajime-san
Copy link
Contributor

Maybe Deno 3 or Deno 4 can re-add the window global, if the world is ready for that, but very unfortunately it is not ready yet.

This is a good idea.
I genuinely want to support josephrocca's opinion, but I also think that Deno need to address many use cases right now. If Deno can reimplement window in the future, that would be great.

@brianjenkins94
Copy link

Another option would be to have the window object be [[IsHTMLDDA]].

That way typeof window === "undefined" but window[...] still works.

@lionel-rowe
Copy link
Contributor

lionel-rowe commented Oct 14, 2024

Another option would be to have the window object be [[IsHTMLDDA]].

Aside from being an "evil hack" this does actually sound like a good solution, with no obvious downside except further sacrificing of technical purity (but technical purity is already acknowledged to be less important than functional code). Probably at least worth testing it out to see if it breaks anything.

Edit: nvm maybe not (due to window === globalThis)

@0f-0b
Copy link
Contributor

0f-0b commented Oct 14, 2024

window === globalThis, but a lot of code depends on typeof globalThis === "object".

@brianjenkins94
Copy link

brianjenkins94 commented Oct 14, 2024

I think this has more to do with older node packages checking typeof window === "undefined" specifically.

globalThis wasn't generally available until 2019-2020.

@petamoriken
Copy link
Contributor

[[IsHTMLDDA]] is specified in Annex B. Furthermore, as its slot name suggests, it's only used exceptionally for document.all. It should not be used in any other way.
tc39/ecma262#673

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

No branches or pull requests

9 participants