lib: add navigator.deviceMemory by Uzlopak · Pull Request #50229 · nodejs/node · GitHub

Shamelessly copied approximation logic from chromium

https://github.com/chromium/chromium/blob/main/third_party/blink/common/device_memory/approximated_device_memory.cc

Review requested:

@nodejs/tsc

I don't think finger-printability is an issue on Node, but I'm not sure about this, so raising this comment just to get some feedback.

At least on spec, this is just a recommendation: https://www.w3.org/TR/device-memory/#computing-device-memory-value

If we do not limit the amount of memory, it will have the same behavior of getTotalMemory, so I don't know if worth to have this function without this behavior of limiting the amount of memory.

Yeah, fingerprinting is not a concern in Node.js. Also, even if it was, why not use os.totalmem()/1024**3 to get the value?

I just ensured that the code is in sync with chrome and according to the documentation/spec.

Suggested change

      
              // Max-limit the reported value to 8GB to reduce fingerprintability of
          
              // high-spec machines.
          
              if (approximated_device_memory_gb_ > 8) approximated_device_memory_gb_ = 8.0;

I prefer we remove this check

navigator is not a good API and I'm still unsure whether it was a good idea to add it at all. The only benefit is interoperability with other JavaScript runtimes, but it seems that neither Safari nor Firefox implement the deviceMemory property. I suppose Google's main goal is to establish the Device-Memory header, but that's unrelated to this PR and generally not very useful for server runtimes.

+1 to what Tobias said, and also I'd be -1 on introducing properties to our implementation of Navigator unless it's listed on the HTML spec (https://html.spec.whatwg.org/multipage/system-state.html#the-navigator-object).

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

Whether or not folks think navigator is a good API in general is not all that material here, in my opinion. I think the more relevant question is whether the new property yields any significant value. The main reason to add anything to navigator is cross-runtime interop and portability. The fact that none of the other runtimes expose navigator.deviceMemory and it is currently only implemented in blink-based browsers rather defeats that argument. I wouldn't say no to adding this but I also don't think there's value is adding it right now.

@jasnell

To be honest, I think nodejs should consider itself as the "brother" of Chrome because both use v8. Bun uses the same engine as Safari, so their behavior is closer than it is to Chrome and nodejs.

Thats why I also directly took the code from the chromium sourcecode, as I would prefer function parity with Chrome.

To be honest, I think nodejs should consider itself as the "brother" of Chrome because both use v8. Bun uses the same engine as Safari, so their behavior is closer than it is to Chrome and nodejs.

Thats why I also directly took the code from the chromium sourcecode, as I would prefer function parity with Chrome.

Why should the choice of using V8 imply anything about feature parity between Node.js and Chrome? This API is not part of V8, not part of the JS language specification, and currently not even part of the HTML specification. Deno, Cloudflare Workers, etc. are also based on V8, and none of them have implemented this API as far as I know.

Implementation is unnecessarily complex

LGTM with the fingerprinting check removed

@targos
implemented it now in javascript according to the spec by using the most significant byte.

@benjamingr
removed the lower and upper bounds.

@nodejs/web-standards
PTAL

I don't think this should land, for the reasons outlined in #50229 (comment).

I also don't understand @Uzlopak's argument that "nodejs should consider itself as the "brother" of Chrome because both use v8" and that one might thus "prefer function parity with Chrome."

My thoughts were, as I stated before, that navigator.deviceMemory is a feature in Chrome. Just want to remind you that implementing the navigator global in node is about browser compatibility. Also this feature has a market share of 75% (chrome 63%, edge 5%, opera 3%, Samsung Internet 2%, etc..).
Personally for me Chrome sets the standard and not Firefox with 3 % or Safari with 19% market share ...

And just because Firefox and Safari didnt implement it, doesnt result in the conclusion that we should not implement it - same as Chrome supporting this feature is not an argument for you.

A good argument against this feature is, that it is still a draft since 2018. Maybe there is a rule already covering these cases

navigator.deviceMemory is also used in few github projects https://github.com/search?q=navigator.deviceMemory&type=code

So we could agree on not implementing a draft spec. Or we can agree on implemeting something which is supported by the most used browser.

I personally dont have a strong opinion on this feature. I just saw a low hanging fruit, so I picked it.

Just want to remind you that implementing the navigator global in node is about browser compatibility

I would argue the point is interoperability, not compatibility.

Since this is a) draft, b) Chrome-only property and so c) one already has to work around its lack of presence in other browsers and runtimes if they wish to have interoperable code, I don't think we should be landing this.

Allrighty. :)

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 18, 2023

Uzlopak force-pushed the add-navigator-deviceMemory branch from 0e2a9ed to edb63e3 Compare October 18, 2023 01:48

H4ad reviewed Oct 18, 2023

View reviewed changes

RafaelGSS added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 18, 2023

tniessen added the experimental Issues and PRs related to experimental features. label Oct 19, 2023

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

ljharb requested changes Oct 28, 2023

View reviewed changes

Comment thread lib/internal/navigator.js Outdated

Uzlopak mentioned this pull request Oct 29, 2023

lib: use primordials for navigator.userAgent #50467

Merged

Uzlopak requested review from H4ad, ljharb and targos October 29, 2023 13:44

targos suggested changes Oct 29, 2023

View reviewed changes

benjamingr approved these changes Oct 29, 2023

View reviewed changes

Uzlopak force-pushed the add-navigator-deviceMemory branch 2 times, most recently from 0aa3941 to 169c7d0 Compare October 30, 2023 09:02

tniessen requested changes Oct 30, 2023

View reviewed changes

Uzlopak force-pushed the add-navigator-deviceMemory branch from 169c7d0 to 0a3899c Compare October 30, 2023 09:59

lib: add navigator.deviceMemory

6c99fee

Uzlopak force-pushed the add-navigator-deviceMemory branch from 0a3899c to 6c99fee Compare October 30, 2023 10:05

panva requested changes Oct 31, 2023

View reviewed changes

Uzlopak closed this Oct 31, 2023

	// Max-limit the reported value to 8GB to reduce fingerprintability of
	// high-spec machines.
	if (approximated_device_memory_gb_ > 8) approximated_device_memory_gb_ = 8.0;

lib: add navigator.deviceMemory by Uzlopak · Pull Request #50229 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: add navigator.deviceMemory - #50229

lib: add navigator.deviceMemory#50229Uzlopak wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom Uzlopak:add-navigator-deviceMemoryUzlopak/node:add-navigator-deviceMemoryCopy head branch name to clipboard

Conversation

Uzlopak commented Oct 18, 2023

Uh oh!

nodejs-github-bot commented Oct 18, 2023

Uh oh!

H4ad Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

targos Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uzlopak Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamingr Oct 29, 2023

Choose a reason for hiding this comment

Uh oh!

tniessen commented Oct 18, 2023

Uh oh!

aduh95 commented Oct 18, 2023

Uh oh!

GeoffreyBooth commented Oct 26, 2023

Uh oh!

Uh oh!

jasnell commented Oct 28, 2023

Uh oh!

Uzlopak commented Oct 29, 2023

Uh oh!

tniessen commented Oct 29, 2023

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Oct 30, 2023

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Oct 30, 2023

Uh oh!

panva left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Oct 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

lib: add navigator.deviceMemory#50229
Uzlopak wants to merge 1 commit into
nodejs:mainfrom
Uzlopak:add-navigator-deviceMemory

H4ad Oct 18, 2023 •
edited
Loading

Uzlopak Oct 18, 2023 •
edited
Loading

panva left a comment •
edited
Loading