-
Notifications
You must be signed in to change notification settings - Fork 32
Description
Environment
Package version:
Deno:
- version:
1.38.2 - OS: Windows
Reproduction
In a Deno REPL, run the following:
import {isNode, isDeno} from 'https://esm.sh/std-env';
console.log(isNode, isDeno); // false trueimport {isNode, isDeno} from 'npm:std-env';
console.log(isNode, isDeno); // true trueDescribe the bug
Under Deno, when using the package in a "regular" way, the platform detection works as expected.
But when using the npm: specifier, isNode is set to true.
If you log the process object exported by the package, you can see the difference: package loaded using URL import has an empty one, while the one loaded with npm has a "regular Node.js" process object, leading to the false positive.
Additional context
It would be feasible to correct that false positive by checking if isDeno is true, then forcing isNode to false. I guess it would be required under Bun as well since it provides Node.js compatibility too.
However, that leads to a question of semantics: do you want the library to report an identity, or a compatibility? Flags starting with is suggest identity, hence my opinion of thinking of this as a bug. However, you could extend the features of your package by also adding flags for compatibility, e.g. isLikeNode, etc.
However the latter would be very tricky: in my tests, globalThis.process, which you use for detection, is undefined except inside the context of evaluation of the package loaded with the npm: specifier. So the package could report a positive Node.js compatibility, when it's not 100% the case: the user has to import the process object here with import process from 'node:process', which is still making a difference. Besides that, Deno embeds Node.js compatibility but is more strict, for instance the node: specifier in imports is required.
I believe it should be double checked for Bun too. EDIT: I checked Bun (on WSL) and it really pushed Node.js' compatibility further: not only process is always global (and process.release.name does equal "node"), but it also doesn't force using node: specifier when importing builtin Node.js modules. And I confirm it also reports isNode as true.