lib: add navigator.deviceMemory

[Uzlopak]

Shamelessly copied approximation logic from chromium

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

[nodejs-github-bot]

Review requested:

• @nodejs/tsc

[H4ad]

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.

[targos]

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?

[Uzlopak]

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

[benjamingr]

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

[tniessen]

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.

[aduh95]

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

[GeoffreyBooth]

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

[jasnell]

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.

[Uzlopak]

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

[tniessen]

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.

[targos]

Implementation is unnecessarily complex

[benjamingr]

LGTM with the fingerprinting check removed

[Uzlopak]

@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

[tniessen]

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

[Uzlopak]

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.

[panva]

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.

[Uzlopak]

Allrighty. :)