lib: brand-check Navigator getters to throw ERR_INVALID_THIS

[3zrv]

Fixes: #63513

Add an explicit #field in this brand check to each getter so a proper ERR_INVALID_THIS is thrown, matching browser behavior. This also fixes language, which previously did not error at all on the prototype because it did not touch a private field.

[nodejs-github-bot]

Review requested:

• @nodejs/web-standards

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.32%. Comparing base (2cdea86) to head (19c4154).
⚠️ Report is 49 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #63601     +/-   ##
=========================================
  Coverage   90.32%   90.32%             
=========================================
  Files         730      732      +2     
  Lines      234669   236681   +2012     
  Branches    43946    44577    +631     
=========================================
+ Hits       211967   213792   +1825     
- Misses      14417    14595    +178     
- Partials     8285     8294      +9     

Files with missing lines	Coverage Δ
lib/internal/navigator.js	99.39% <100.00%> (+0.01%)	⬆️

... and 85 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[aduh95]

I don't think adding a check is worth it, I think the current behavior is fine actually, the spec requires a TypeError, we're throwing a TypeError (except for .language, indeed that's worth fixing). For reference, Chromium-based browsers throw TypeError: Illegal invocation

[LiviaMedeiros]

+1 on explicit ERR_INVALID_THIS over implicit private field access. Errors from private fields are misleading and not future-proof: it's too easy to break 'implicit throw' unintentionally (in fact, Navigator.prototype.language was initially introduced using private field, too).

[aduh95]

Errors from private fields are misleading and not future-proof: it's too easy to break 'implicit throw' unintentionally

It’s only true if that’s not covered by tests (which I would have expected WPT to cover, but I guess not, or at least not the subset we’re running)

[KhafraDev]

In undici we are lenient with errors too with the expectation that a reviewer would notice if a change similar to the one that changed .language was made.

Since it's not reasonable here, I think the change is fine. Regarding the WPTs, brand checks should be implemented the same across globals in most platforms so the assumption is that if you have it implemented properly in one spec, it's also implemented correctly in the thousands of over browser globals.

[3zrv]

Thanks for the reviews, the test now covers all getters via Object.keys(Navigator.prototype), so either approach is safe against future regressions of the .language kind.

I'm more into keeping the explicit ERR_INVALID_THIS checks for to maintain error message quality, instead of returning a internal field name, ERR_INVALID_THIS('Navigator') produces the same shape of message Chromium and FireFox emit, and matches what the rest of Node's brand-checked APIs throw

[aduh95]

Here's what the error looks like on the browser side:

• Safari: TypeError: The Navigator.language getter can only be used on instances of Navigator
• Firefox: TypeError: 'get language' called on an object that does not implement interface Navigator.
• Chromium: TypeError: Illegal invocation

FWIW I'm in the Chromium camp, I think minimal effort is the appropriate amount of support we should spend on "passing an invalid this" use-case, so I'm not keen on adding a if that will slow down the happy path, ever so slightly – but it's certainly not a hill I want to die on for Navigator

[3zrv]

are we still good to merge this? missing anything? @aduh95 @KhafraDev @LiviaMedeiros

[anonrig]

We shouldn't do this. We explicitly avoided doing this because of performance in the past. We went through deprecation cycles multiple times because of this.

[LiviaMedeiros]

Wouldn't not throwing here go against the specs?

[anonrig]

Accessing this.#attribute already throws

[LiviaMedeiros]

This particular getter doesn't use private field anymore so currently it doesn't throw at all.

[aduh95]

@anonrig you can try it yourself: node -p Navigator.prototype.language exits with code 0 (while accessing the other properties e.g. node -p Navigator.prototype.languages has the expected TypeError)

[aduh95]

Suggested change

	if (!(#languages in this)) throw new ERR_INVALID_THIS('Navigator');
	this.#languages; // eslint-disable-line no-unused-expressions

[anonrig]

@aduh95 your suggestion is the correct approach

[LiviaMedeiros]

LGTM. I'd still prefer if all errors were aligned, but as long as this approach doesn't preclude potential follow-up and is easier to reach consensus for, looks great.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/73875/