-
Notifications
You must be signed in to change notification settings - Fork 25
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
Using package in Bun or in Deno with npm:
specifier reports isNode
as true
#100
Comments
npm:
specifier reports isNode
as true
npm:
specifier reports isNode
as true
Hi, and thanks for writing a detailed issue. In Node.js compatibility mode, I guess Deno tries to mimic Node.js env and set I understand that you want to strictly be sure about current runtime and this is a really ecosystem wide confusing issue. It will be tricky to decide for Bun and Deno what expect in compatibility mode to be detected-as, they like libraries to magically assume them Node or not. We probably need to reach out. It is also a breaking change for What i suggest to do, is to introduce some new strict exports like |
When entering that mode, which is the case when loading packages with the
It's more about how this projects decides to define the semantics of the flags it computes. Then consumers of the API will simply follow what's documented.
Right now I see this library as doing identity checking rather than duck testing, so that's why I was personally expecting the flags to tell what the platform truly is (also because of the naming scheme of the flags). Duck testing would be more complex and demanding, but if done correctly would manage to identify that Deno is not Node.js compatible out of the box (AFAIU) while Bun is.
That, I think, is an open debate. "breaking change" refers to semantic versioning, and here we are potentially talking about a bug fix, which is not considered as a breaking change in this context. But when thinking more concretely, yes it is a breaking change. However we can always find a way to consider "any" change a breaking change since it modifies the context of a program. Changing the content of the code you distribute is a breaking change if a consumer, for any reason at all, decides to read the raw code and do some things about it. That sounds silly but why not. However, the precise layout of the distributed code is not considered as part of the public contract for this project, hence why no one considers that as a breaking change. "breaking change" or not should always be considered with regards to the explicit contract, or in other words the public documentation, and therefore the semantics of the API. In that case, if you always defined the platform detection as being strict (i.e. "it is" vs "it is like/compatible with"), then we're talking about a bug fix. A fix that would indeed break the programs of the consumers if they were relying on that bug. But I personally really doubt projects should start considering cases like that otherwise it becomes a nightmare to handle and all releases would be a major version bump.
Not sure I understand the suggestion here. So |
Hi dear @ymeine thanks again for bringing this up. Thinking more and considering the situation that more runtimes are deciding to have a Node.js compatibility mode, I think it is finally up to them to make sure their runtimes are Node.js compatible when they explicitly say We have a new fix for Bun runtime check (#107) and I have also clarified docs/jsdocs about For strict Node.js checking |
Hi, thanks for the update and the fixes! I think the documentation clarification is the essential part so it's good that it's here now. If ever the behavior would change again (due to updates external to this project), people should now be more alert about what may have cause it. I personally still think the name |
Environment
Package version:
3.5.0
Deno:
1.38.2
Reproduction
In a Deno REPL, run the following:
Describe 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 forcingisNode
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, isundefined
except inside the context of evaluation of the package loaded with thenpm:
specifier. So the package could report a positive Node.js compatibility, when it's not 100% the case: the user has to import theprocess
object here withimport process from 'node:process'
, which is still making a difference. Besides that, Deno embeds Node.js compatibility but is more strict, for instance thenode:
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 (andprocess.release.name
does equal"node"
), but it also doesn't force usingnode:
specifier when importing builtin Node.js modules. And I confirm it also reportsisNode
astrue
.The text was updated successfully, but these errors were encountered: