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

Neutral JavaScript runtime support (Deno, Bun, etc) #2169

Closed
sgammon opened this issue Mar 31, 2024 · 11 comments · Fixed by #2170
Closed

Neutral JavaScript runtime support (Deno, Bun, etc) #2169

sgammon opened this issue Mar 31, 2024 · 11 comments · Fixed by #2170

Comments

@sgammon
Copy link
Contributor

sgammon commented Mar 31, 2024

Hey there @tj,

First of all, big fan of commander; I've used it for a long time, so, longtime user first time contributor 😁. I have a patch for commander which makes it work neutrally on all JS runtimes; it was already working fine on Bun and Node, but with the following two small changes:

  • Prefixing Node API imports with node:*
  • Importing process into an explicit symbol

... it will now run smoothly on Deno as well.

For context: I'm trying to ship a tool downstream, hashlock, which uses commander, but I want it to be able to run on any JS runtime.

Distribution diff:

diff --git a/node_modules/commander/lib/command.js b/node_modules/commander/lib/command.js
index 5b16e60..e1ddedb 100644
--- a/node_modules/commander/lib/command.js
+++ b/node_modules/commander/lib/command.js
@@ -1,8 +1,8 @@
-const EventEmitter = require('events').EventEmitter;
-const childProcess = require('child_process');
-const path = require('path');
-const fs = require('fs');
-const process = require('process');
+const EventEmitter = require('node:events').EventEmitter;
+const childProcess = require('node:child_process');
+const path = require('node:path');
+const fs = require('node:fs');
+const process = require('node:process');
 
 const { Argument, humanReadableArgName } = require('./argument.js');
 const { CommanderError } = require('./error.js');

PR incoming shortly.

This issue body was partially generated by patch-package.

sgammon added a commit to sgammon/commander.js that referenced this issue Mar 31, 2024
Fixes and closes tj#2169 by using Node-prefixed imports.

Signed-off-by: Sam Gammon <sam@elide.dev>
@shadowspawn
Copy link
Collaborator

shadowspawn commented Mar 31, 2024

Using the node prefix is probably fine, but I haven't reproduced a requirement to do so in a simple program. What failure do you see? (I haven't tried reproducing using your full package yet.)

I can run a simple program with Deno without code changes:

import { Command } from "npm:commander";
const program = new Command();
program.parse();
% deno run --allow-read prefixed.ts --help 
Usage: prefixed [options] <target>

Options:
  -d, --debug
  -h, --help   display help for command

It also works if I import without the npm prefix and have commander installed/added locally:

import { Command } from "commander";

sgammon added a commit to sgammon/hashlock that referenced this issue Mar 31, 2024
- feat: support all popular js runtimes
  - patches for `glob`, `minipass`, and `path-scurry`
  - upstream prs (listed below)
  - test entrypoint commands
- test: add test entrypoints for each major runtime
- test: add scripts to test entrypoint with each major runtime
- chore: sync lockfiles

Related Issues
- isaacs/node-glob#580
- isaacs/path-scurry#16
- isaacs/minipass#54
- tj/commander.js#2169

Upstream PRs
- isaacs/node-glob#581
- isaacs/minipass#55
- isaacs/path-scurry#17
- tj/commander.js#2170

Signed-off-by: Sam Gammon <sam@elide.dev>
sgammon added a commit to sgammon/hashlock that referenced this issue Mar 31, 2024
- feat: support all popular js runtimes
  - patches for `glob`, `minipass`, and `path-scurry`
  - upstream prs (listed below)
  - test entrypoint commands
- test: add test entrypoints for each major runtime
- test: add scripts to test entrypoint with each major runtime
- chore: sync lockfiles

Related Issues
- isaacs/node-glob#580
- isaacs/path-scurry#16
- isaacs/minipass#54
- tj/commander.js#2169

Upstream PRs
- isaacs/node-glob#581
- isaacs/minipass#55
- isaacs/path-scurry#17
- tj/commander.js#2170

Signed-off-by: Sam Gammon <sam@elide.dev>
sgammon added a commit to sgammon/hashlock that referenced this issue Mar 31, 2024
- feat: support all popular js runtimes
  - patches for `glob`, `minipass`, and `path-scurry`
  - upstream prs (listed below)
  - test entrypoint commands
- test: add test entrypoints for each major runtime
- test: add scripts to test entrypoint with each major runtime
- chore: sync lockfiles

Related Issues
- isaacs/node-glob#580
- isaacs/path-scurry#16
- isaacs/minipass#54
- tj/commander.js#2169

Upstream PRs
- isaacs/node-glob#581
- isaacs/minipass#55
- isaacs/path-scurry#17
- tj/commander.js#2170

Signed-off-by: Sam Gammon <sam@elide.dev>
sgammon added a commit to sgammon/hashlock that referenced this issue Mar 31, 2024
* feat: support all runtimes

- feat: support all popular js runtimes
  - patches for `glob`, `minipass`, and `path-scurry`
  - upstream prs (listed below)
  - test entrypoint commands
- test: add test entrypoints for each major runtime
- test: add scripts to test entrypoint with each major runtime
- chore: sync lockfiles

Related Issues
- isaacs/node-glob#580
- isaacs/path-scurry#16
- isaacs/minipass#54
- tj/commander.js#2169

Upstream PRs
- isaacs/node-glob#581
- isaacs/minipass#55
- isaacs/path-scurry#17
- tj/commander.js#2170

Signed-off-by: Sam Gammon <sam@elide.dev>

* chore: version bump → `1.0.3`

Signed-off-by: Sam Gammon <sam@elide.ventures>

---------

Signed-off-by: Sam Gammon <sam@elide.dev>
Signed-off-by: Sam Gammon <sam@elide.ventures>
@sgammon
Copy link
Contributor Author

sgammon commented Mar 31, 2024

@shadowspawn In my Commander app, when I run:

deno --version && pnpm run entry:deno --help

# entry:deno is:
# deno run --allow-read ./path/to/commander/entrypoint/file.mjs

I get:

deno 1.40.5 (release, aarch64-apple-darwin)
v8 12.1.285.27
typescript 5.3.3

> hashlock@1.0.3 entry:deno /Volumes/VAULTROOM/verify-hashes
> deno run --allow-read ./dist/cli.mjs "--help"

error: Relative import path "fs/promises" not prefixed with / or ./ or ../
If you want to use a built-in Node module, add a "node:" prefix (ex. "node:fs/promises").
    at file:///Volumes/VAULTROOM/verify-hashes/dist/cli.mjs:6825:52
 ELIFECYCLE  Command failed with exit code 1.

Deno latest is 1.42.0, so I upgraded and ran it again. Same result. Inspecting the built entrypoint file, I see a few of these unqualified imports specifically in commander/lib/commander.js

Screenshot 2024-03-30 at 9 14 48 PM

I can't address these with my build system - esbuild - because it does not rewrite imports in node_modules on my behalf. I looked there before filing.

I can try importing commander with npm:*, but I'm hoping not to apply dynamic import behavior per-runtime if I can avoid it, only because it will make it harder to maintain my library (also, I'm not sure how this changes support semantics across other runtimes). I'm happy to try though if it would produce helpful diagnosis material.

Applying the enclosed patch fixes the issue for me and it runs smoothly as expected.

@shadowspawn
Copy link
Collaborator

I would like to see this for myself to make sure what problem is being resolving. If I understand correctly, after bundling your application with esbuild you get runtime errors when using Deno to run the application. The example you give shows fs/promises but that is not used by Commander. I expect the problem is similar for the dependencies actually used by Commander. What steps can I take to perform a build so I can reproduce the error? (I can hack out the patch, I have failed so far to do a build.)

git clone git@github.com:sgammon/hashlock.git
cd hashlock
????

@shadowspawn
Copy link
Collaborator

First of all, big fan of commander; I've used it for a long time, so, longtime user first time contributor 😁.

❤️

@sgammon
Copy link
Contributor Author

sgammon commented Mar 31, 2024

@shadowspawn

I would like to see this for myself to make sure what the problem is being [resolved].

No worries! That is reasonable.

The example you give shows fs/promises but that is not used by Commander

Yes, there are some other libs I am using which have this same issue (I have posted PRs for those too). For Commander, it is the builtin modules events, child_process, path, and fs. process also needs to be imported so there is a formal symbol for it.

What steps can I take to perform a build so I can reproduce the error?

git clone git@github.com:sgammon/hashlock.git
cd hashlock
nvm use 20
pnpm install --ignore-scripts

👆 Ignoring scripts will skip the patch-package step and install Commander without modification. Then:

pnpm run build
pnpm run entry:node --help
pnpm run entry:bun --help
pnpm run entry:deno --help

Each run entry:* command tests with a different runtime. You should get a valid --help response for Node and Bun, but then Deno fails.

@shadowspawn
Copy link
Collaborator

shadowspawn commented Mar 31, 2024

I get multiple errors when I try the build. (On main branch.) The first error is for glob which is a little interesting given it is one of the files which would get patched. The build also fails after doing a plain install with the patches applied.

Building on Mac (Apple Silicon), in case that makes a difference.

% bun --version
1.0.36
% pnpm --version
8.15.5
% node --version
v20.11.1
% pnpm run build

> hashlock@1.0.3 build /Users/john/Documents/Sandpits/commander/issues/2169/hashlock
> bun ./scripts/build.mjs && bun run build:standalone && bun run build:types && cp -fv ./dist/index.d.ts ./dist/index.d.mts

Building 'verify-hashes'...
- Building 'hashlock' (lib, cjs)...
- Building 'hashlock' (CLI, esm)...
- Building 'hashlock' (action)...
✘ [ERROR] Could not resolve "glob"

    src/main.ts:19:21:
      19 │ import { glob } from 'glob'
         ╵                      ~~~~~~

  The Yarn Plug'n'Play manifest forbids importing "glob" here because it's not listed as a
  dependency of this package:
...

@sgammon
Copy link
Contributor Author

sgammon commented Apr 1, 2024

@shadowspawn That is odd--it should not be using Yarn PNP. I use 'pnpm install'

@sgammon
Copy link
Contributor Author

sgammon commented Apr 1, 2024

I'm away from my machine but I will look into this shortly. From the top of my head

  • Our node, bun, and pnpm versions seem aligned

  • I'm not using PNP at all

  • I am using Corepack so pnpm should be pinned

  • The place where it fails in the build is related to the action compile in CJS, I think? I can't immediately tell. But in any case the CLI built fine in the log you have provided.

All told you can probably run "entry:*" whether the build passes or not, because the entry points there are pure TypeScript and thus don't need to be built.

@shadowspawn
Copy link
Collaborator

shadowspawn commented Apr 1, 2024

But in any case the CLI built fine in the log you have provided.

The Building lines are from before the async builds, and all the builds fail (to be clear, I did truncate the log). No files are being generated in .dist/, and in particular not ./dist/cli.mjs.

I tried downloading a build package to examine the build files, but got an error for that too and didn't pursue further:

% npm install hashlock
npm ERR! code 127
npm ERR! path /Users/john/Documents/Sandpits/commander/issues/2169/bin/node_modules/hashlock
npm ERR! command failed
npm ERR! command sh -c patch-package
npm ERR! sh: patch-package: command not found

Edit: installed hashlock with --ignore-scripts to look at dist/cli.mjs.

@shadowspawn
Copy link
Collaborator

shadowspawn commented Apr 1, 2024

I assume the transpile/bundle is effectively removing the implicit context of Commander code being in an npm module due to discovery via package.json, so then the import of an unprefixed node packages causes an error in Deno.

https://deno.com/blog/package-json-support

@sgammon
Copy link
Contributor Author

sgammon commented Apr 2, 2024

@shadowspawn Yes, I think that's right. When it runs in CLI form it is fully bundled; the standalone executable also bundles this code.

I really can't say why you're getting the PNP issue... I can prepare a smaller repro, or perhaps a devcontainer? glob should be installed as a regular Node dependency, so it should be present in node_modules/glob. Is it present for you after running pnpm install --ignore-scripts?

I don't build with Deno, I just want the final target to be able to run under it. Thanks again for helping me diagnose this. I'd be happy to prepare a smaller repro in a project or devcontainer as well, maybe that can get around any env issues.

@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Apr 7, 2024
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label May 18, 2024
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 a pull request may close this issue.

2 participants