node
node copied to clipboard
nodejs
lib: add `navigator.hardwareConcurrency`
I'm following up on an abandoned pull request, and adding authors as the co-author to this pull request.
I've simplified and only added navigator.hardwareConcurrency right now, to receive feedback, understand any blockers before investing more time on it.
Referencing issue: https://github.com/nodejs/node/issues/39540 Follow up of the pull request: https://github.com/nodejs/node/pull/39581
navigator.platform is a deprecated API. If we are adding the Navigator interface we probably should start with something that's not deprecated...The value of navigator.platform is also unspecified, I wonder how useful this could be if we are still returning something different from what browsers do (and then browsers also disagree among themselves).
If we don't do navigator.platform (which is fine) ... let's start with navigator.userAgent, which is defined as part of the WinterCG minimal (https://common-min-api.proposal.wintercg.org/)
navigator.platform isn't deprecated in the spec, only on MDN. I'm pretty sure it's only deprecated there to discourage people from using it (see the note). Also +1 on starting with or adding userAgent.
As I said in the original PR and in multiple related issues, I still think most of the Navigator interface makes little sense for Node.js. It's part of the HTML spec and only exposed there on the window object, which happens to correspond to the global scope.
I don't think we should be adding navigator to Node.js. The majorjty of its API are very browser specific that does not match Node.js use case.
I don't have any issues in the readonly properties or navigator, but I'm failing to understand the use case of only limiting to those.
@mcollina I didn't understand. Are you blocking this PR because it includes navigator or because of navigator.platform? Will you unblock if it only included navigator.userAgent stated in the WinterCG Minimum Common Web Platform API?
The HTML spec https://html.spec.whatwg.org/multipage/system-state.html#client-identification states that:
Warning: Any information in this API that varies from user to user can be used to profile the user. In fact, if enough such information is available, a user can actually be uniquely identified. For this reason, user agent implementers are strongly urged to include as little information in this API as possible.
Is there any use case for Node.js to implement a less precise API when more accurate APIs are available to retrieve this information?
MDN documentation say:
A Navigator object can be retrieved using the read-only window.navigator property.
IMHO Node does not have a window object, so it should not provide a Navigator object via globalThis.
window is synonymous with globalThis, globalThis.navigator is the same as window.navigator with a few more characters.
@mcollina:
I don't think we should add navigator to Node.js
I rarely disagree with Matteo but in this case I have to ;-) There are several APIs exposed via navigator that are or could be relevant to Node.js, for instance:
-
navigator.hardwareConcurrency -
navigator.locks -
navigator.userAgent -
navigator.userAgentData -
navigator.sendBeacon() -
navigator.gpu
One could fairly easily come up with very valid use cases for these in Node.js.
I converted the pull request to use a less debatable attribute, in order to introduce navigator object. Appreciate reviewing it again @mcollina and @nodejs/tsc
@nodejs/tsc I've added a tsc-agenda label, to ask for concensus regarding navigator global class, and later resolve or close this pull request.
Is there any expectation that if one part of the api is included that other parts will be as well?
I was thinking that if we land this, we should (of course) do it in a semver major, but also mark it as experimental to signal that we might remove it if it causes breakage. (Removing it otherwise would also be semver major.) I know this gets into the whole "experimental features that aren't behind a flag aren't really experimental" discussion, but I want to signal that we might remove it in a patch or minor release, and labeling it experimental seems to be the way to do that.
@cjihrig noted that Deno already has navigator so using the presence of navigator to detect if you're running in a browser doesn't work if you support Deno. That mitigates my concern about adding navigator breaking code for people who use its presence to detect if they are in a browser or not.
@MoLow mentioned that we should definitely run CITGM before landing this (which we are supposed to do for all semver major PRs) and I certainly agree with that.
I was thinking that if we land this, we should (of course) do it in a semver major, but also mark it as experimental to signal that we might remove it if it causes breakage.
And let’s please start using the experimental stages for all new experimental features: https://nodejs.org/api/documentation.html#stability-index. That would better inform users how we feel about the API.
If it’s a low stage, that might suggest we should add a flag or at least a warning.
@nodejs/platform-windows Can you help me identify the issue behind the failing tests on node-test-commit-windows-fanned?
Dear reviewers, appreciate a review to land this pull request. cc @mcollina @ronag
The https://github.com/nodejs/node/labels/notable-change label has been added by @anonrig.
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}