Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion lib/internal/navigator.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const {

const {
ERR_ILLEGAL_CONSTRUCTOR,
ERR_INVALID_THIS,
} = require('internal/errors').codes;

const {
Expand Down Expand Up @@ -79,7 +80,9 @@ function getNavigatorPlatform(arch, platform) {
}

class Navigator {
// Private properties are used to avoid brand validations.
// Private properties also serve as a brand check via the `#field in obj`
// operator in each getter, so callers get a proper TypeError instead of
// an internal "private field" error when invoked on a non-Navigator `this`.
#availableParallelism;
#locks;
#userAgent;
Expand All @@ -97,6 +100,7 @@ class Navigator {
* @returns {number}
*/
get hardwareConcurrency() {
if (!(#availableParallelism in this)) throw new ERR_INVALID_THIS('Navigator');
Comment thread
aduh95 marked this conversation as resolved.
Outdated
this.#availableParallelism ??= getAvailableParallelism();
return this.#availableParallelism;
}
Expand All @@ -105,6 +109,7 @@ class Navigator {
* @returns {LockManager}
*/
get locks() {
if (!(#locks in this)) throw new ERR_INVALID_THIS('Navigator');
this.#locks ??= require('internal/locks').locks;
return this.#locks;
}
Expand All @@ -113,6 +118,7 @@ class Navigator {
* @returns {string}
*/
get language() {
if (!(#languages in this)) throw new ERR_INVALID_THIS('Navigator');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wouldn't not throwing here go against the specs?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Accessing this.#attribute already throws

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@aduh95 your suggestion is the correct approach

// The default locale might be changed dynamically, so always invoke the
// binding.
return getDefaultLocale() || 'en-US';
Expand All @@ -122,6 +128,7 @@ class Navigator {
* @returns {Array<string>}
*/
get languages() {
if (!(#languages in this)) throw new ERR_INVALID_THIS('Navigator');
this.#languages ??= ObjectFreeze([this.language]);
return this.#languages;
}
Expand All @@ -130,6 +137,7 @@ class Navigator {
* @returns {string}
*/
get userAgent() {
if (!(#userAgent in this)) throw new ERR_INVALID_THIS('Navigator');
this.#userAgent ??= `Node.js/${StringPrototypeSlice(nodeVersion, 1, StringPrototypeIndexOf(nodeVersion, '.'))}`;
return this.#userAgent;
}
Expand All @@ -138,6 +146,7 @@ class Navigator {
* @returns {string}
*/
get platform() {
if (!(#platform in this)) throw new ERR_INVALID_THIS('Navigator');
this.#platform ??= getNavigatorPlatform(arch, platform);
return this.#platform;
}
Expand Down
30 changes: 30 additions & 0 deletions test/parallel/test-navigator.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,3 +147,33 @@ Object.defineProperty(navigator, 'language', { value: 'for-testing' });
assert.strictEqual(navigator.language, 'for-testing');
assert.strictEqual(navigator.languages.length, 1);
assert.strictEqual(navigator.languages[0] !== 'for-testing', true);

{
const { Navigator } = globalThis;
const getterNames = [
'hardwareConcurrency',
'locks',
'language',
'languages',
'userAgent',
'platform',
];
for (const name of getterNames) {
Comment thread
3zrv marked this conversation as resolved.
Outdated
assert.throws(
() => Navigator.prototype[name],
{
name: 'TypeError',
code: 'ERR_INVALID_THIS',
},
`expected TypeError when reading ${name} on Navigator.prototype`,
);
assert.throws(
() => Reflect.get(Navigator.prototype, name, {}),
{
name: 'TypeError',
code: 'ERR_INVALID_THIS',
},
`expected TypeError when reading ${name} on a plain object`,
);
}
}
Loading