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

lib: add navigator.platform #50385

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Oct 25, 2023

adds navigator.platform

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 25, 2023
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 25, 2023

@zm-cttae
@anonrig

@Uzlopak Uzlopak force-pushed the navigator-platform branch from 0667117 to 42b169f Compare October 25, 2023 11:57
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, while this is somewhat standardized, even Deno hasn't implemented it, and the top of the MDN page clearly discourages its use:

This feature is no longer recommended. Though some browsers might still support it, it may have already been removed from the relevant web standards, may be in the process of being dropped, or may only be kept for compatibility purposes. Avoid using it, and update existing code if possible (..). Be aware that this feature may cease to work at any time.

I suppose this PR is in line with the option to "add more ancient HTML properties" as outlined in #50269 (comment), so if that's the direction the project is going... 🤷

(FWIW, it would also be spec-compliant to return some useless/incorrect value or potentially even an empty string.)

doc/api/globals.md Show resolved Hide resolved
@tniessen tniessen added experimental Issues and PRs related to experimental features. semver-minor PRs that contain new features and should be released in the next minor version. labels Oct 25, 2023
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 25, 2023

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

@tniessen
Copy link
Member

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

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 26, 2023

@anonrig
@aduh95

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

@GeoffreyBooth GeoffreyBooth added the web-standards Issues and PRs related to Web APIs label Oct 26, 2023
@GeoffreyBooth
Copy link
Member

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.

@GeoffreyBooth
Copy link
Member

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

@GeoffreyBooth
Copy link
Member

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?

@tniessen
Copy link
Member

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.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 26, 2023

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.

@GeoffreyBooth
Copy link
Member

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.

@nodejs-github-bot
Copy link
Collaborator

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 27, 2023

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.

@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth
Copy link
Member

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.

@zm-cttae
Copy link

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

@Uzlopak Uzlopak force-pushed the navigator-platform branch 2 times, most recently from 961a62f to 66e2ffc Compare October 28, 2023 12:57
@jasnell
Copy link
Member

jasnell commented Oct 28, 2023

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?

@zm-cttae
Copy link

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

@nodejs-github-bot
Copy link
Collaborator

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 2, 2023

@GeoffreyBooth

image

Please merge :)

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 2, 2023

Or maybe @H4ad ?

@H4ad H4ad added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Nov 2, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 2, 2023
@nodejs-github-bot nodejs-github-bot merged commit a450eed into nodejs:main Nov 2, 2023
22 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in a450eed

@Uzlopak Uzlopak deleted the navigator-platform branch November 2, 2023 20:21
anonrig pushed a commit to anonrig/node that referenced this pull request Nov 9, 2023
PR-URL: nodejs#50385
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #50385
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos added a commit that referenced this pull request Nov 12, 2023
Notable changes:

doc:
  * add MrJithil to collaborators (Jithil P Ponnan) #50666
  * add Ethan-Arrowood as a collaborator (Ethan Arrowood) #50393
esm:
  * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) #48740
fs:
  * add stacktrace to fs/promises (翠 / green) #49849
lib:
  * (SEMVER-MINOR) add navigator.platform (Aras Abbasi) #50385
stream:
  * (SEMVER-MINOR) add support for `deflate-raw` format to webstreams compression (Damian Krzeminski) #50097
  * use Array for Readable buffer (Robert Nagy) #50341
  * optimize creation (Robert Nagy) #50337
test_runner:
  * adds built in lcov reporter (Phil Nash) #50018
  * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) #48638
test_runner, cli:
  * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) #50443

PR-URL: #50681
targos added a commit that referenced this pull request Nov 13, 2023
Notable changes:

doc:
  * add MrJithil to collaborators (Jithil P Ponnan) #50666
  * add Ethan-Arrowood as a collaborator (Ethan Arrowood) #50393
esm:
  * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) #48740
fs:
  * add stacktrace to fs/promises (翠 / green) #49849
lib:
  * (SEMVER-MINOR) add `--no-experimental-global-navigator` CLI flag (Antoine du Hamel) #50562
  * (SEMVER-MINOR) add navigator.language & navigator.languages (Aras Abbasi) #50303
  * (SEMVER-MINOR) add navigator.platform (Aras Abbasi) #50385
stream:
  * (SEMVER-MINOR) add support for `deflate-raw` format to webstreams compression (Damian Krzeminski) #50097
  * use Array for Readable buffer (Robert Nagy) #50341
  * optimize creation (Robert Nagy) #50337
test_runner:
  * (SEMVER-MINOR) adds built in lcov reporter (Phil Nash) #50018
  * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) #48638
test_runner, cli:
  * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) #50443

PR-URL: #50681
targos pushed a commit that referenced this pull request Nov 14, 2023
PR-URL: #50385
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos added a commit that referenced this pull request Nov 14, 2023
Notable changes:

doc:
  * add MrJithil to collaborators (Jithil P Ponnan) #50666
  * add Ethan-Arrowood as a collaborator (Ethan Arrowood) #50393
esm:
  * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) #48740
fs:
  * add stacktrace to fs/promises (翠 / green) #49849
lib:
  * (SEMVER-MINOR) add `--no-experimental-global-navigator` CLI flag (Antoine du Hamel) #50562
  * (SEMVER-MINOR) add navigator.language & navigator.languages (Aras Abbasi) #50303
  * (SEMVER-MINOR) add navigator.platform (Aras Abbasi) #50385
stream:
  * (SEMVER-MINOR) add support for `deflate-raw` format to webstreams compression (Damian Krzeminski) #50097
  * use Array for Readable buffer (Robert Nagy) #50341
  * optimize creation (Robert Nagy) #50337
test_runner:
  * (SEMVER-MINOR) adds built in lcov reporter (Phil Nash) #50018
  * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) #48638
test_runner, cli:
  * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) #50443

PR-URL: #50681
targos added a commit that referenced this pull request Nov 14, 2023
Notable changes:

doc:
  * add MrJithil to collaborators (Jithil P Ponnan) #50666
  * add Ethan-Arrowood as a collaborator (Ethan Arrowood) #50393
esm:
  * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) #48740
fs:
  * add stacktrace to fs/promises (翠 / green) #49849
lib:
  * (SEMVER-MINOR) add `--no-experimental-global-navigator` CLI flag (Antoine du Hamel) #50562
  * (SEMVER-MINOR) add navigator.language & navigator.languages (Aras Abbasi) #50303
  * (SEMVER-MINOR) add navigator.platform (Aras Abbasi) #50385
stream:
  * (SEMVER-MINOR) add support for `deflate-raw` format to webstreams compression (Damian Krzeminski) #50097
  * use Array for Readable buffer (Robert Nagy) #50341
  * optimize creation (Robert Nagy) #50337
test_runner:
  * (SEMVER-MINOR) adds built in lcov reporter (Phil Nash) #50018
  * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) #48638
test_runner, cli:
  * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) #50443

PR-URL: #50681
martenrichter pushed a commit to martenrichter/node that referenced this pull request Nov 26, 2023
Notable changes:

doc:
  * add MrJithil to collaborators (Jithil P Ponnan) nodejs#50666
  * add Ethan-Arrowood as a collaborator (Ethan Arrowood) nodejs#50393
esm:
  * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) nodejs#48740
fs:
  * add stacktrace to fs/promises (翠 / green) nodejs#49849
lib:
  * (SEMVER-MINOR) add `--no-experimental-global-navigator` CLI flag (Antoine du Hamel) nodejs#50562
  * (SEMVER-MINOR) add navigator.language & navigator.languages (Aras Abbasi) nodejs#50303
  * (SEMVER-MINOR) add navigator.platform (Aras Abbasi) nodejs#50385
stream:
  * (SEMVER-MINOR) add support for `deflate-raw` format to webstreams compression (Damian Krzeminski) nodejs#50097
  * use Array for Readable buffer (Robert Nagy) nodejs#50341
  * optimize creation (Robert Nagy) nodejs#50337
test_runner:
  * (SEMVER-MINOR) adds built in lcov reporter (Phil Nash) nodejs#50018
  * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) nodejs#48638
test_runner, cli:
  * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) nodejs#50443

PR-URL: nodejs#50681
@richardlau richardlau added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. experimental Issues and PRs related to experimental features. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. web-standards Issues and PRs related to Web APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants