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: Use node-prefixed requires for builtins #2170

Merged
merged 1 commit into from
Apr 6, 2024

Conversation

sgammon
Copy link
Contributor

@sgammon sgammon commented Mar 31, 2024

Pull Request

Problem

I want to use a Commander-based CLI tool from Deno, as explained in #2169

Solution

Fixes and closes #2169 by using Node-prefixed imports.

Note: Node-prefixed imports are supported on Node 16+, so this may break older apps on very old versions of Node. As a result I would suggest a major version bump to signal a compatibility change. Node 16 has reached EOL, so people really should be upgrading if they can.

No code was changed in Commander itself; just imports.

ChangeLog

  • Use node:-prefixed requires; this may break compatibility with older versions of Node (up to 15). Please upgrade Node, bundle your app with rewriting to remove the node: prefix on requires, or avoid upgrading Commander if Node cannot be upgraded.

Fixes and closes tj#2169 by using Node-prefixed imports.

Signed-off-by: Sam Gammon <sam@elide.dev>
Comment on lines -1 to +5
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');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the whole pr

sgammon added a commit to sgammon/hashlock that referenced this pull request 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 pull request 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 pull request 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 pull request 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>
@shadowspawn
Copy link
Collaborator

Commander v12 requires node v18 or higher, so node compatibility for using the node: protocol is not an issue.

"node": ">=18"

@shadowspawn
Copy link
Collaborator

shadowspawn commented Apr 6, 2024

This issue only shows up because of bundling then running with Deno, and the change to fix it is only likely to break similar workflows involving transpiling and bundling tools not supporting the "node:" prefix for built-in modules.

The node: prefix for core modules has been available in Node.js since (at least) v16.0.0

So I think normal uses of Commander won't be affected by this change, and if it breaks some more exotic uses then we will roll it back.

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

LGTM

@abetomo abetomo changed the base branch from master to develop April 6, 2024 23:57
Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

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

👍

@abetomo abetomo merged commit f7b8475 into tj:develop Apr 6, 2024
8 checks passed
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Apr 7, 2024
sgammon added a commit to sgammon/hashlock that referenced this pull request Apr 15, 2024
Adopt changes from tj/commander.js#2170

Signed-off-by: Sam Gammon <sam@elide.ventures>
sgammon added a commit to sgammon/hashlock that referenced this pull request Apr 15, 2024
* fix: postinstall script

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

* chore: version bump → `1.0.4`

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

* chore: update to commander.js `develop` branch

Adopt changes from tj/commander.js#2170

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

* fix: disable distcheck (tmp)

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

* chore: update all node deps

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

---------

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

Released in v12.1.0

@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label May 18, 2024
@sgammon
Copy link
Contributor Author

sgammon commented May 19, 2024

@shadowspawn Thank you for the ping! I'll go ahead and adopt. 🥳

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.

Neutral JavaScript runtime support (Deno, Bun, etc)
3 participants