refactor(path): make isWindows check compatible with Node and Bun

[BenMcLean981]

Description

As described by the linked issue the isWindows function @std/path is not cross runtime. In it's existing state it should work on Deno and the browser (though there are issues there) but not bun or node. In this PR I believe I have resolved those issues and added support for bun or node.

Tested Runtimes

I manually tested this solution in the following runtimes:

• Deno on Fedora Linux
• Firefox on Fedora Linux
• Node on Fedora Linux
• Bun on Fedora Linux
• Deno on Windows 10
• Firefox on Windows 10
• Node on Windows 10
• Bun on Windows 10

I couldn't think of an easy way to test this automatically. I could probably write a bash script to do it so that it can be added to the pipeline if necessary.

Potential Issues with Existing Browser Support

As mentioned in the discussion there may have been issues with the existing implementation for browsers. The navigator.appVersion property is deprecated, so I moved away from that and started using navigator.userAgent.

Bun and Node Support

I got Bun and Node working using Node's os module which also has an implementation in bun. The code for this is pretty ugly, as require does not exist in Deno or the browser. I could not think of a better way to do this.

Resolves #4914

[kt3k]

If node.js is in ESM mode (.mjs or type: module in package.json), then it doesn't have require.

[BenMcLean981]

Ok, I tested by manually converting to js and then running with node os.js, explains why this worked for me then.

I'll mark this PR as Draft again until I can come up with a better solution.

[kt3k]

Thanks.

@std is published in JSR, and it's distributed as ESM in Node.js. So I think it's enough to only support ESM context.

[iuioiua]

This change can be far more easily made using navigator.userAgent, which Node and Bun support. For Bun, you can check navigator.userAgent.includes("Bun"). Same with Node.

[codecov]

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 96.26%. Comparing base (745c4a6) to head (aabaca9).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
path/_os.ts	57.14%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4961   +/-   ##
=======================================
  Coverage   96.26%   96.26%           
=======================================
  Files         479      479           
  Lines       38705    38700    -5     
  Branches     5617     5619    +2     
=======================================
- Hits        37259    37256    -3     
+ Misses       1403     1401    -2     
  Partials       43       43           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[BenMcLean981]

I noticed that while I was testing. But that doesn't allow us to check if the underlying os is Windows (which is the only thing this module does that's public facing).

[iuioiua]

On second thought, we could do the following:

1. Determine whether the current runtime is a browser or not (Deno, Bun, or Node).
2. If in the browser, use navigator.userAgent to check if on Windows.
3. If not a browser, dynamically import node:os to check if on Windows. That should take care of Deno, Bun and Node.

WDYT?

[kt3k]

If not a browser, dynamically import node:os to check if on Windows. That should take care of Deno, Bun and Node.

I'm not in favor of this as this introduces Top-level await and makes the module async. We saw many issues about async modules in the past..

Node.js (>=21) and Bun seem exposing navigator.platform property, which seems usable for OS detection. https://nodejs.org/api/globals.html#navigatorplatform

[BenMcLean981]

I also considered this and I don't like the idea of using async/await in any way. I'm going to look and see how the node os module works under the hood and see if maybe we can't just do it the same way they do.

[BenMcLean981]

I poked around in node's os module and saw how they check to see if node is running on windows. They just use process.platform, which we of course can do here as well. I've tested it and it seems to work.

[iuioiua]

Ah, yes, I forgot about top-level await being problematic. I think using navigator.platform is the best method. But we need to implement it in the runtime (see denoland/deno#24117).

[BenMcLean981]

@iuioiua for this PR that isn't necessary. I was able to do it with process.platform in node and bun. In the browser and Deno I have this wrapped in a try/catch. This worked on all of the runtimes I listed in the PR description.

[iuioiua]

Sorry for the delayed review, @BenMcLean981. I've simplified your logic to just use Deno.build.os or navigator.platform, depending on the runtime. This should all be compatible with Deno, Bun, Node and the browser.

Thank you very much for your contribution! @kt3k, can you PTAL and be sure to update the compatibility of @std/path on JSR once the next release is cut?

[kt3k]

I changed the commit type from feat to refactor as it's not tested with Node and Bun yet.

[iuioiua]

I don't see how this PR, in it's current state, not yet being tested on Node or Bun makes this a refactor PR. A package being made available in two new runtimes is a feature. @BenMcLean981, is there any chance you'd be able to test this PR, in its current state, on the various platforms/runtimes once more?

[kt3k]

We can't ask somebody to test on every change. We need to set up CI to ensure it's working on specific platform.

[iuioiua]

I can confirm this works on macOS (POSIX) for Deno, Bun, Windows and Chrome.

[kt3k]

I'd suggest we would make feat(path): add support of Node commit when we set up the CI on Node.

[kt3k]

LGTM

[iuioiua]

While I disagree with this being a refactor PR, I can at least understand the reasoning. I'd like to get confirmation that this works on Windows before merging.

[kt3k]

I changed the title a bit as there seem somethings remaining (like the usage of Deno.cwd()) to make @std/path truly compatible with Node

[BenMcLean981]

@iuioiua I ran my little set of manual tests again on my Windows machine. It doesn't work on Node. An error gets thrown that navigator is undefined, if I use a nullish check on it then it incorrectly returns false. I added in a branch to the conditional check to use process.platform for the check, all is well now.

I agree with the discussion around cross-platform and cross-runtime automated testing. I was thinking about setting that up for this repo, but I think that needs to be part of a larger discussion. I'm also not sure what tooling even exists to make that happen.

[kt3k]

Ah ok. navigator.platform seems only available on Node >= 21.2 https://nodejs.org/api/globals.html#navigatorplatform

It would be better to support lower versions like 18.x, 20.x, as they are still officially supported versions.

[iuioiua]

I think we should support from the LTS version (now v20.17.0), upwards. So the recent change makes sense. Thanks, @BenMcLean981 for confirming and checking.

[BenMcLean981]

I think we should support from the LTS version (now v20.17.0), upwards. So the recent change makes sense. Thanks, @BenMcLean981 for confirming and checking.

No problem! Happy I could contribute.