GeezerVison

@zm-cttae
@anonrig

The MDN is not a reliable source regarding the deprecation of those properties. They only mark it as deprecated because they can not mark something as "avoid as best as you can".

Also it is not It is not spec compliant to return an empty string. See whatwg/html#7762

Even Typescript considers the deprecation as a bug:
microsoft/TypeScript#49683

See also for more insights:
mdn/content#14429

It is part of whatwg testing:
https://github.com/web-platform-tests/wpt/blob/72e06c82933e9d324881b756a3fc48a3cac1c09d/workers/interfaces/WorkerUtils/navigator/004.js

The MDN is not a reliable source regarding the deprecation of those properties. They only mark it as deprecated because they can not mark something as "avoid as best as you can".

I did not use the word "deprecation". If the intended meaning is "avoid as best as you can", as you put it, that is precisely what I am referring to. Node.js applications worked just fine without this property for far more than a decade.

It is not spec compliant to return an empty string.

You are right, the spec was changed a while ago. Instead, it is spec-compliant to return an incorrect value now...

@anonrig
@aduh95

Can someome please retrigger macos build? The build error should be independent from this PR.

The MDN is not a reliable source regarding the deprecation of those properties. They only mark it as deprecated because they can not mark something as “avoid as best as you can”.

I skimmed the referenced links but I can’t find an answer to why MDN wants to discourage it. Reading https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform it seems like they discourage its use simply because they recommend using feature detection rather than platform detection; is that your guess as well?

I think for Node’s case, sure feature detection is always preferable whenever possible but we have plenty of use cases that absolutely need platform detection, such as choosing a particular binary to run. We don’t discourage the use of process.platform, after all. So I think it would be good for us to add this, to give developers a cross-runtime way of doing platform detection.

Let’s please ping @nodejs/web-standards for all issues and PRs related to navigator.

Yes, it seems to be very similar to #50385 (comment). In either case, it’s not clear why MDN is intending to discourage the use of this API, other than perhaps because they think feature detection is a better approach in general. Is that your impression or is there some other reason?

it seems like they discourage its use simply because they recommend using feature detection rather than platform detection; is that your guess as well?

@GeoffreyBooth I think that's the main idea, but the spec also allows browser to return entirely incorrect values for privacy, so developers should not rely on navigator.platform in any case.

Reading https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform it seems like they discourage its use simply because they recommend using feature detection rather than platform detection; is that your guess as well?

Yes, I think so too.

Btw. I am willing to add more attributes, but some stuff seems odd, like navigator.appName is always 'Navigator' for compatibility reasons. So should it be also added? So yes, I hope we get to an agreement soon, so that we can finish this object as best as possible.

I think we can keep adding properties regardless of what other PRs are floating out there. No one has put up a PR to fully delete the global, just to flag it, so even if it gets flagged it should still have whatever properties we think it should have.

I’m inclined to add platform and really any other property that makes even remote sense, because it’s better to have a property exist than not, for code that might break when checking for it and assuming its existence.

CI: https://ci.nodejs.org/job/node-test-pull-request/55271/

Hmm. I consider to work again on this in few hours.

creating a new function called getNavigatorPlatform, which expects an optional parameter of shape { platform, arch }, which by default gets process.
Then I can test it by providing mocks into the function and having 100% test coverage.

CI: https://ci.nodejs.org/job/node-test-pull-request/55276/

Then I can test it by providing mocks into the function and having 100% test coverage.

I don’t think this is necessary. Our CI already runs the test suite on every supported platform, so every branch of the if is already getting checked, albeit not in the same run.

Ideally that last assertion, for the miscellaneous Linux platforms, would assert against plain strings but it would be a pain to figure out what all of those should be. You could assert against a temporary string like ‘TODO’ and I can run CI and you can look at the CI output on each failing platform to see what the string could be for that platform, and then we can put that in here. I’m not sure it’s worth the effort though.

It appears that platform is also a valid part of the spec.

This one I'm genuinely not convinced we should export. As has been pointed out in discussion there are a number of issues with this, not the least of which being that even browsers recommend against relying on it. I note that bun implements it but deno doesn't. That said, a quick code search on github shows 66k JS files using this for various purposes so perhaps it is worthwhile? @Uzlopak ... can you expand a bit on the use case you have in mind for this?

The reason it was implemented was because Next.js relies on the prop when Navigator is available globally.

CI: https://ci.nodejs.org/job/node-test-pull-request/55429/

@GeoffreyBooth

Please merge :)

Or maybe @H4ad ?

Landed in a450eed