| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
The notable-change PRs with changes that should be highlighted in changelogs. 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. |
Sorry, something went wrong.
|
Two questions:
|
Sorry, something went wrong.
Some packages might depend on navigator.userAgent to detect if it's not Node.js or not. I don't want to introduce breaking change.
navigator.userAgent is included in WINTERCG Minimum Common Web Platform API - https://common-min-api.proposal.wintercg.org |
Sorry, something went wrong.
|
The default value contained in this PR is currently different from the value used in native fetch, see: nodejs/undici#2306 There is also some discussion about not including the version number that may be of relevance here. I think if Node.js exposes a user agent in multiple places, it needs to be the same everywhere. |
Sorry, something went wrong.
|
the version should be removed, so either "node.js" or "node". Undici uses "node" so that would make our lives easier (the fetch user-agent and navigator user-agent seem like they should be equal, although I'm not entirely sure). |
Sorry, something went wrong.
Referencing WINTERCG, it's recommended to have a product version with the user agent. I'm more in favor of having the version: User-Agent = product *( RWS ( product / comment ) ) product = token ["/" product-version] product-version = token |
Sorry, something went wrong.
|
There are legitimate security concerns by adding the node version, maybe something like what Chrome is doing would be acceptable? If you don't want to click that they're replacing minor version numbers with zeros (ie. v20.1.1 -> v20.0.0). Is there any discussion about why a version number is required? |
Sorry, something went wrong.
|
I thought about the version. I am +1 for adding the version. First of all, the userAgent is in browsers used for requests. We dont expose the version via a http request nor via a http response. Browsers expose this information with their version via request and they dont consider it as a security issue. If it would be a security issue chrome, safari and co would have removed the version information long time ago. Also comparing to deno and bun, they also expose the version. So I am 100% hoping that the version gets exposed. |
Sorry, something went wrong.
Actually Chrome only exposes the major version; they consider exposing the minor/patch versions to be a security concern. See the Chrome/118.0.0.0 in the example above. All of the Chrome 118.x versions return 118.0.0.0 as the version. But we can do the same, it’s better than not including a version. |
Sorry, something went wrong.
|
Chrome did limit it, from the link I posted above:
Firefox also limits some information (windows & macOS versions are capped at 10 and 10.15 respectively, and some architectures don't include any information in their user-agent header). (Source) I'd assume they have plans to limit it further. |
Sorry, something went wrong.
Yep, this was my proposed alternative. |
Sorry, something went wrong.
|
I'll update the PR with the proposed change. |
Sorry, something went wrong.
|
This isn’t semver-major, the semver-major change was adding navigator in the first place. Now that navigator exists we should add all the properties people expect on it. |
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
RSLGTM
Sorry, something went wrong.
💯 agree! Also something to consider: it looks like there is existing code out there that looks for Node.js string matching. For example: |
Sorry, something went wrong.
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
|
I really think the user-agent keys should match #50200 (comment) Can we update it ? |
Sorry, something went wrong.
|
honestly i'm starting to question the benefit of having navigator whatsoever. I think there should be an agreement on if node should even have it before it's changed again. |
Sorry, something went wrong.
PR-URL: #50200 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Notable changes: doc: * add H4ad to collaborators (Vinícius Lourenço) #50217 esm: * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096 fs: * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095 lib: * (SEMVER-MINOR) add `navigator.userAgent` (Yagiz Nizipli) #50200 stream: * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187 * call helper function from push and unshift (Raz Luvaton) #50173 PR-URL: #50335
Notable changes: doc: * add H4ad to collaborators (Vinícius Lourenço) #50217 esm: * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096 fs: * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095 lib: * (SEMVER-MINOR) add `navigator.userAgent` (Yagiz Nizipli) #50200 stream: * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187 * call helper function from push and unshift (Raz Luvaton) #50173 PR-URL: #50335
Notable changes: doc: * add H4ad to collaborators (Vinícius Lourenço) #50217 esm: * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096 fs: * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095 lib: * (SEMVER-MINOR) add `navigator.userAgent` (Yagiz Nizipli) #50200 stream: * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187 * call helper function from push and unshift (Raz Luvaton) #50173 PR-URL: #50335
Notable changes: doc: * add H4ad to collaborators (Vinícius Lourenço) #50217 esm: * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096 fs: * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095 lib: * (SEMVER-MINOR) add `navigator.userAgent` (Yagiz Nizipli) #50200 stream: * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187 * call helper function from push and unshift (Raz Luvaton) #50173 PR-URL: #50335
| Back | FazBrowse Home | New Git URL |
Follow-up for #47769
cc @nodejs/tsc