Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use platform over appversion on global navigator object #5389

Open
wants to merge 7 commits into
base: production
Choose a base branch
from

Conversation

melton-jason
Copy link
Contributor

This feature is no longer recommended. Though some browsers might still support it, it may have already been removed from the relevant web standards, may be in the process of being dropped, or may only be kept for compatibility purposes. Avoid using it, and update existing code if possible; see the compatibility table at the bottom of this page to guide your decision. Be aware that this feature may cease to work at any time.

From Mozilla on appVersion: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/appVersion

Recently, localization tests began failing because of an invalid setting of global.navigator:

https://github.com/specify/specify7/actions/runs/11707985658/job/32609369992

global.navigator = {
userAgent: 'node.js',
appVersion: 'node.js',
platform: 'node.js',
};

This failing test was remedied in 76871c7, which functionally completely removed the reassignment of global.navigator.

However, it appears that support for navigator.appVersion has been dropped in some capacity, as navigator.appVersion was returning undefined:

https://github.com/specify/specify7/actions/runs/11726874323/job/32666586609

const altKeyName = globalThis.navigator?.appVersion.includes('Mac')
? 'Option'
: 'Alt';

This PR cleans up the code in the JSDOM configuration file, and uses navigator.platform instead of appVersion.

⚠️ Note that navigator.platform is itself deprecated: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform

However, this use case is explicitly mentioned in the Mozilla docs, and may be a better alternative until a better solution is found...

But there is one case where, among the options you could use, navigator.platform may be the least-bad option: When you need to show users advice about whether the modifier key for keyboard shortcuts is the ⌘ command key (found on Apple systems) rather than the ⌃ control key (on non-Apple systems)

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)

@melton-jason melton-jason marked this pull request as ready for review November 7, 2024 20:40
@melton-jason melton-jason requested a review from a team November 7, 2024 20:40
Comment on lines 5 to 8
* FEATURE: Allow customizing the JSDOM userAgent, platform, and other global
* properties
* See https://github.com/jsdom/jsdom#advanced-configuration
* See https://github.com/specify/specify7/pull/5389
Copy link
Member

Choose a reason for hiding this comment

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

that should probably be done using mocking:

in beforeAll in some test file set userAgent/platform/...
in afterAll undo the change

* detection for browsers using Mac.
* See https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform
*/
const altKeyName = globalThis.navigator?.platform.startsWith('Mac')
Copy link
Member

Choose a reason for hiding this comment

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

this is exactly the use case that was quoted by you from the MDN article, so probably using navigator.platform in the right choice in this case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📋Back Log
Development

Successfully merging this pull request may close these issues.

3 participants