-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
lib: hide navigator
behind a flag
#50412
Conversation
Review requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm -1 on this. It was a change included within a semver major release. Reverting this at this late, would not help with adoption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m also -1. It arguably is complete now that we have navigator.userAgent
, as that’s the only property listed in the WinterCG minimum: https://common-min-api.proposal.wintercg.org/#requirements-for-navigatoruseragent. We don’t need to and might not add any more properties, which makes “far from complete” a bit of a strange thing to say.
Comparing us to Bun and Deno, we have the two properties in common with both other runtimes: #50269 (comment).
Also this flag is unnecessary. I already posted instructions on how to remove the global if desired: #50310. Since the API is already potentially complete, it’s better that it’s enabled by default and users can disable it rather than the reverse.
Please tag @nodejs/web-standards on issues/PRs related to Web APIs.
My goal is not to help adoption, but limit ecosystem breakage. I think having this enabled by default does more harm than good – i.e. my impression is that the subset of users who needs/benefits from the presence of this API is much smaller than the subset that's being disrupted by it. Putting behind a flag lets folks who benefit from it keep using it, without disrupting the others. |
I don't think telling people to do a multi-decade bad practice - modify objects they don't own, which includes deleting globals - is something that's appropriate to recommend under any circumstances. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Navigator API is still very far from complete
What does "complete" mean here? Node will never be compliant with the spec (ie. navigator plugins or cookies), nor will node match what other WinterCG compliant environments do because afaict they all support different things.
@GeoffreyBooth unfortunately @nodejs/web-standards wasn't pinged on #50310, which seems like a problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's experimental and if people believe it's not ready or has consensus I think hiding it is a good step while discussion is had
That's fair, "complete" is not well defined, and not exactly relevant, I should not have used it. I've updated the OP to deal with the more concrete problem some people have with this API: is it relevant for Node.js and its users? The answer may very well be yes, but from what I read it's not obvious to everyone. And indeed, some folks would still prefer to not have this API no matter how "complete" its implementation would be. |
(And note, this is only to mitigate breakage, people are running into this in v21 even-though most people are probably on v20 or v18 - I believe our users that this is real breakage and we should discuss it once we've resolved it for them) |
As I said in yesterday’s TSC meeting, we made two mistakes:
I think if My biggest objection to this PR is that there’s no end date where the flag ever gets removed. Now that we have And there is value in |
The breakage is caused by exposing it as a global; could perhaps instead of requiring a flag, it move to a requireable/importable module? That way node can get feedback, and it'll be exposed by default, with no risk of breakage while the kinks are worked out. |
Presumably, when we've assessed ecosystem breakage, made a reasonable attempt to contact authors of big'ish libraries that break because of this change and let them know, communicated with our users and tried to restore their trust after we've caused them breakage. None of these steps are particularly hard and I'm optimistic |
In a way, shipping the break was the outreach; we heard from the affected libraries in #50269. And all of the ones we heard from are fixed by 21.1.0, because we added But also, 21.0.0 was a semver major release of Node. It’s allowed to and supposed to include breaking changes. And this one wasn’t hidden: it’s one of the notable changes in the release notes. I don’t think it’s “breaking trust” of users to ship a breaking change in a semver-major release. And better now, in a non-LTS release, than in 22. |
Rather than one-off flags like The downside would be that some code would be written expecting the global to exist and might break when this flag was used, but at least this way any potentially problematic global is under the user’s control and the user can be responsible for using a mix of dependencies that work with whatever settings the user runs Node under. |
I don't think that's the issue with navigator; the issue is that it's a bad api and discussing if it even has a place in node. |
I think the advantage of this option is that we keep most of the disruption in the 21.x line, which would reduce the impact of 22.x. There are presumably lots of people who will upgrade straight from 20 to 22, so the more that any breakage is discovered and fixed during 21.x, the less that will break when people start using 22. |
I don't think this is a good idea, it's going to create git conflicts (at least in the history section of the docs). Why not land this and then do a semver-major follow up that switches the flag? |
Seems like apache echarts has more issues. But i would rather provide a PR to that project and make it compatible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion this API was landed too early. I still don't know if I agree we should have this at all. Node.js and WinterCG compliance has not been agreed upon afaik, so simply for that sake we should be more hesitant to be adding WinterCG related APIs without further consideration.
Tracing the error cited on that issue, it seems like it boils down to this: else if (typeof navigator === 'undefined') {
// In node
env.node = true; Which . . . isn’t a bug in Node. There isn’t some future version of the |
True, but it is a reasonable assumption for code to have made that a non-browser will never have |
Or let’s go with option a2 and replace this PR with one that adds a |
@GeoffreyBooth That is precisely the kind of breakage I predicted in #50412 (comment). These checks exist and they are reasonable based on standards and conventions from far more than a decade. This already seems to have inspired undesirable workarounds, such as emulating a DOM because Node.js pretends to be a browser (see apache/echarts#19233 (comment)).
It may be your personal opinion that such code is "flawed." However, as multiple people have explained here and elsewhere, a navigator was almost universally understood as a web browser (and referred to as such in the relevant standards) until, very recently, a few server runtimes decided to break that convention. I don't think the Node.js project should blame the ecosystem for any breakage here. |
Yes, and it’s what @jasnell was saying about “inherently brittle” code and what I was describing as “anticipated” breakage. Quibbling over whether I’m justified in using the word “flawed” to describe such code is a distraction. Want to call it “based upon a completely reasonable assumption”? Sure, fine, I don’t care. It doesn’t matter what we think about it. What matters is where we go from here. There is no future version of the Node
The other options don’t really make sense in my view:
I think we need to give our users a bit more credit here. They understand that semver-major releases of Node have breaking changes, and that some work on the user’s side might be required as a result, but that the benefit is new functionality that wouldn’t have been possible otherwise. Adding |
There is of course a 5th option, expose it unflagged, just not as a global. That way, anyone who wants to write universal code can do so explicitly with a small fallback, and no code risks being broken. |
I really @ljharb option. |
I fixed the issue in echarts. |
Web workers have a navigator as well and it doesn't navigate and doesn't have access to Given that the issues (#50412 (comment)) are fixed by the addition of |
It’s only broken there for the extreme minority of code that’s run in a web worker. worker_threads would be analogous, but regular node is where the risk of breakage is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed my mind on this.
Unless other major breakages shows up, I think we can keep it.
I think in hindsight we probably should've done a runtime deprecation of first access to |
FYI I have opened an alternative in #50562 which adds a flag but keep the global by default (I.e. with the other PR it’s opt-out, while this one was opt-in). I figured opening a new PR was more logical as those who reviewed this PR may have very different opinions on the other one, happy to get your reviews, hopefully we can get consensus on either PR (and it doesn’t look it’s going to happen on that one). |
How so? The issue you linked to was closed as completed three months ago and hasn’t had any activity since. |
Hey, unfortunately it still exists. Error
Environment and package details
Almost exactly the same thing that was described in the original issue. Prior to this update, I had no issue with sass. |
The current latest version of the 21.x line is 21.5.0. It might very well fix your issue. A comment on a related issue on the |
Thanks for your help and patience. Super thank you 🙏🏿 |
The relevance of Navigator API in Node.js has not been established, and exposing it early has caused breakage in the ecosystem. IMO we should not expose it by default, not until there's consensus around it.