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

fix(env): fix env detection compatibility in node v21.1.0 #1036

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Nov 2, 2023

This PR improves the detection of nodejs version 21.1.0 and above

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 2, 2023

I personally would have preferred

if (typeof process !== 'undefined' && process.versions && typeof node.versions.node === 'string')

@plainheart plainheart changed the title fix: detect node21.1.0 fix(env): fix env detection compatibility in node v21.1.0 Nov 2, 2023
Copy link
Collaborator

@plainheart plainheart left a comment

Choose a reason for hiding this comment

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

@plainheart plainheart linked an issue Nov 2, 2023 that may be closed by this pull request
@plainheart plainheart merged commit 401e314 into ecomfe:master Nov 2, 2023
@Uzlopak Uzlopak deleted the node-21 branch November 2, 2023 10:00
@plainheart
Copy link
Collaborator

@Uzlopak A small question: Before v21.1.0, is the navigator.userAgent undefined? Will it bring an NPE? Do we need to add the existence check?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 2, 2023

For what stands the abbreviation NPE?

@plainheart
Copy link
Collaborator

Null Pointer Exception - that is, a type error thrown when accessing properties / methods of an undefined object.

@plainheart
Copy link
Collaborator

I looked through the release note of Node v21.0.0, which added the navigator.hardwareConcurrency property. So in that version, navigator.userAgent may be undefined.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 2, 2023

In node 21.0.0 userAgent was missing. But node 21 is anyway a dev version and it was expected to result in regressions. So using it for production is anyway bad.

I would personally try to detect node/deno/bun with

if ((typeof process !== 'undefined' && typeof process.version === 'string') || 'Deno' in window)

first one would detect node and bun, second one would detect Deno.

@tniessen
Copy link

tniessen commented Nov 2, 2023

But node 21 is anyway a dev version and it was expected to result in regressions. So using it for production is anyway bad.

FWIW, that is not the official stance of the Node.js project. While we generally recommend LTS versions for production, non-LTS release lines are not "dev versions". They typically receive the same level of support as LTS versions, albeit for a shorter amount of time. Breaking changes may be introduced in both LTS and non-LTS release lines, but regressions are highly undesirable in any case.

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

Successfully merging this pull request may close these issues.

[Bug] Does not work using Node 21.0.0
3 participants