fix: allows ESM modules commands to be extensible using visit option#2468
Merged
bcoe merged 3 commits intoyargs:mainfrom May 6, 2025
Merged
fix: allows ESM modules commands to be extensible using visit option#2468bcoe merged 3 commits intoyargs:mainfrom
bcoe merged 3 commits intoyargs:mainfrom
Conversation
bcoe
reviewed
May 2, 2025
lib/command.ts
Outdated
| const joined = this.shim.path.join(fullDirPath, file); | ||
| const module = req(joined); | ||
| const visited = visit(module, joined, file); | ||
| const extendableModule = {...module}; |
Member
There was a problem hiding this comment.
nit, let's make this have a null prototype.
const extendableModule = Object.create(null, Object.getOwnPropertyDescriptors({... module}));
Contributor
Author
There was a problem hiding this comment.
const extendableModule = Object.create(null, Object.getOwnPropertyDescriptors({... module}));
This code will let the extendableModule lose its prototype, it causes another test to fail.
Test: it('supports a "visit" function option')
commandObject.should.have.property('command')
TypeError: Cannot read properties of undefined (reading 'have')
Because chai use should() to put should assertions into Object.prototype.
So what do you think?
Member
There was a problem hiding this comment.
In the failing 'supports a "visit" function option' test, replace the pattern foo.should() with expect(foo).to.
https://www.chaijs.com/guide/styles/
e.g.
+ commandObject.should.have.property('builder');
- expect(commandObject).to.have.property('builder');
Contributor
Author
There was a problem hiding this comment.
Current test is already this style
commandObject.should.have
.property('command')
.and.equal('dream [command] [opts]');
commandObject.should.have
.property('desc')
.and.equal('Go to sleep and dream');
commandObject.should.have.property('builder');
commandObject.should.have.property('handler');The problem is
const extendableModule = Object.create(null, Object.getOwnPropertyDescriptors({... module}));
This makes extendableModule a null prototype, it makes chai assertion mechanism logic fail.
I am not sure why null prototype is important in this case. Actually, the change works, but it makes chai fail.
I have tried this change to make the test pass
it('supports a "visit" function option', () => {
let commandObject;
let pathToFile;
let filename;
const r = checkOutput(() =>
yargs('--help')
.wrap(null)
.commandDir('fixtures/cmddir', {
visit(_commandObject, _pathToFile, _filename) {
commandObject = _commandObject;
pathToFile = _pathToFile;
filename = _filename;
return false; // exclude command
},
})
.parse()
);
+ Object.setPrototypeOf(commandObject, Object.prototype);
commandObject.should.have
.property('command')
.and.equal('dream [command] [opts]');
commandObject.should.have
.property('desc')
.and.equal('Go to sleep and dream');
commandObject.should.have.property('builder');
commandObject.should.have.property('handler');I have committed the code. Please take a look.
Member
There was a problem hiding this comment.
Sorry, I got the diff example in the wrong order. Switch from should to expect for this test, which does not require mucking with prototypes.
- commandObject.should.have.property('builder');
+ expect(commandObject).to.have.property('builder');
Member
There was a problem hiding this comment.
.should and expect() support the same chaining language. should uses extended Object prototype, while expect is a plain function.
(My impression is that extending prototype was popular in the past, but is not the preferred approach in modern applications.)
Member
There was a problem hiding this comment.
When we are only using an object as a collection of properties, we can get extra safety by using a null-prototype. This avoids collecting any extra properties either from JavaScript itself (like .toString) or added by other code (like .should().
In this case it might be safe without the null prototype, but Yargs has had problems in past leading to CVE from casual use of object without a null prototype. So paranoid, and easy to be robust.
Contributor
Author
There was a problem hiding this comment.
Got it and done. Please take a look.
shadowspawn
approved these changes
May 4, 2025
Member
shadowspawn
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the new test, and updating the old test.
Member
|
@vipzhicheng thank you for the contribution. |
renovate bot
added a commit
to andrei-picus-tink/auto-renovate
that referenced
this pull request
Jun 1, 2025
| datasource | package | from | to | | ---------- | ------- | ------ | ------ | | npm | yargs | 17.7.2 | 18.0.0 | ## [v18.0.0](https://github.com/yargs/yargs/blob/HEAD/CHANGELOG.md#1800-2025-05-26) ##### ⚠ BREAKING CHANGES - command names are not derived from modules passed to `command`. - singleton usage of yargs yargs.foo, yargs().argv, has been removed. - minimum node.js versions now `^20.19.0 || ^22.12.0 || >=23`. - yargs is now ESM first ##### Features - commandDir now works with ESM files ([#2461](yargs/yargs#2461)) ([27eec18](yargs/yargs@27eec18)) - **locale:** adds hebrew translation ([#2357](yargs/yargs#2357)) ([4266485](yargs/yargs@4266485)) - yargs is now ESM first ([d90af45](yargs/yargs@d90af45)) - **zsh:** Add default completion as fallback ([#2331](yargs/yargs#2331)) ([e02c91b](yargs/yargs@e02c91b)) ##### Bug Fixes - addDirectory do not support absolute command dir ([#2465](yargs/yargs#2465)) ([3a40a78](yargs/yargs@3a40a78)) - allows ESM modules commands to be extensible using visit option ([#2468](yargs/yargs#2468)) ([200e1aa](yargs/yargs@200e1aa)) - **browser:** fix shims so that yargs continues working in browser context ([#2457](yargs/yargs#2457)) ([4ae5f57](yargs/yargs@4ae5f57)) - **build:** address problems with typescript compilation ([#2445](yargs/yargs#2445)) ([8d72fb3](yargs/yargs@8d72fb3)) - coerce should play well with parser configuration ([#2308](yargs/yargs#2308)) ([8343c66](yargs/yargs@8343c66)) - **deps:** update dependency yargs-parser to v22 ([#2470](yargs/yargs#2470)) ([639130d](yargs/yargs@639130d)) - exit after async handler done ([#2313](yargs/yargs#2313)) ([e326cde](yargs/yargs@e326cde)) - handle spaces in bash completion ([#2452](yargs/yargs#2452)) ([83b7788](yargs/yargs@83b7788)) - parser-configuration should work well with generated completion script ([#2332](yargs/yargs#2332)) ([888db19](yargs/yargs@888db19)) - propagate Dictionary including undefined in value type ([#2393](yargs/yargs#2393)) ([2b2f7f5](yargs/yargs@2b2f7f5)) - **zsh:** completion no longer requires double tab when using autoloaded ([0dd8fe4](yargs/yargs@0dd8fe4)) ##### Code Refactoring - command names are not derived from modules passed to `command`. ([d90af45](yargs/yargs@d90af45)) - singleton usage of yargs yargs.foo, yargs().argv, has been removed. ([d90af45](yargs/yargs@d90af45)) ##### Build System - minimum node.js versions now `^20.19.0 || ^22.12.0 || >=23`. ([d90af45](yargs/yargs@d90af45))
renovate bot
added a commit
to andrei-picus-tink/auto-renovate
that referenced
this pull request
Jun 6, 2025
| datasource | package | from | to | | ---------- | ------- | ------ | ------ | | npm | yargs | 17.7.2 | 18.0.0 | ## [v18.0.0](https://github.com/yargs/yargs/blob/HEAD/CHANGELOG.md#1800-2025-05-26) ##### ⚠ BREAKING CHANGES - command names are not derived from modules passed to `command`. - singleton usage of yargs yargs.foo, yargs().argv, has been removed. - minimum node.js versions now `^20.19.0 || ^22.12.0 || >=23`. - yargs is now ESM first ##### Features - commandDir now works with ESM files ([#2461](yargs/yargs#2461)) ([27eec18](yargs/yargs@27eec18)) - **locale:** adds hebrew translation ([#2357](yargs/yargs#2357)) ([4266485](yargs/yargs@4266485)) - yargs is now ESM first ([d90af45](yargs/yargs@d90af45)) - **zsh:** Add default completion as fallback ([#2331](yargs/yargs#2331)) ([e02c91b](yargs/yargs@e02c91b)) ##### Bug Fixes - addDirectory do not support absolute command dir ([#2465](yargs/yargs#2465)) ([3a40a78](yargs/yargs@3a40a78)) - allows ESM modules commands to be extensible using visit option ([#2468](yargs/yargs#2468)) ([200e1aa](yargs/yargs@200e1aa)) - **browser:** fix shims so that yargs continues working in browser context ([#2457](yargs/yargs#2457)) ([4ae5f57](yargs/yargs@4ae5f57)) - **build:** address problems with typescript compilation ([#2445](yargs/yargs#2445)) ([8d72fb3](yargs/yargs@8d72fb3)) - coerce should play well with parser configuration ([#2308](yargs/yargs#2308)) ([8343c66](yargs/yargs@8343c66)) - **deps:** update dependency yargs-parser to v22 ([#2470](yargs/yargs#2470)) ([639130d](yargs/yargs@639130d)) - exit after async handler done ([#2313](yargs/yargs#2313)) ([e326cde](yargs/yargs@e326cde)) - handle spaces in bash completion ([#2452](yargs/yargs#2452)) ([83b7788](yargs/yargs@83b7788)) - parser-configuration should work well with generated completion script ([#2332](yargs/yargs#2332)) ([888db19](yargs/yargs@888db19)) - propagate Dictionary including undefined in value type ([#2393](yargs/yargs#2393)) ([2b2f7f5](yargs/yargs@2b2f7f5)) - **zsh:** completion no longer requires double tab when using autoloaded ([0dd8fe4](yargs/yargs@0dd8fe4)) ##### Code Refactoring - command names are not derived from modules passed to `command`. ([d90af45](yargs/yargs@d90af45)) - singleton usage of yargs yargs.foo, yargs().argv, has been removed. ([d90af45](yargs/yargs@d90af45)) ##### Build System - minimum node.js versions now `^20.19.0 || ^22.12.0 || >=23`. ([d90af45](yargs/yargs@d90af45))
renovate bot
added a commit
to andrei-picus-tink/auto-renovate
that referenced
this pull request
Jun 8, 2025
| datasource | package | from | to | | ---------- | ------- | ------ | ------ | | npm | yargs | 17.7.2 | 18.0.0 | ## [v18.0.0](https://github.com/yargs/yargs/blob/HEAD/CHANGELOG.md#1800-2025-05-26) ##### ⚠ BREAKING CHANGES - command names are not derived from modules passed to `command`. - singleton usage of yargs yargs.foo, yargs().argv, has been removed. - minimum node.js versions now `^20.19.0 || ^22.12.0 || >=23`. - yargs is now ESM first ##### Features - commandDir now works with ESM files ([#2461](yargs/yargs#2461)) ([27eec18](yargs/yargs@27eec18)) - **locale:** adds hebrew translation ([#2357](yargs/yargs#2357)) ([4266485](yargs/yargs@4266485)) - yargs is now ESM first ([d90af45](yargs/yargs@d90af45)) - **zsh:** Add default completion as fallback ([#2331](yargs/yargs#2331)) ([e02c91b](yargs/yargs@e02c91b)) ##### Bug Fixes - addDirectory do not support absolute command dir ([#2465](yargs/yargs#2465)) ([3a40a78](yargs/yargs@3a40a78)) - allows ESM modules commands to be extensible using visit option ([#2468](yargs/yargs#2468)) ([200e1aa](yargs/yargs@200e1aa)) - **browser:** fix shims so that yargs continues working in browser context ([#2457](yargs/yargs#2457)) ([4ae5f57](yargs/yargs@4ae5f57)) - **build:** address problems with typescript compilation ([#2445](yargs/yargs#2445)) ([8d72fb3](yargs/yargs@8d72fb3)) - coerce should play well with parser configuration ([#2308](yargs/yargs#2308)) ([8343c66](yargs/yargs@8343c66)) - **deps:** update dependency yargs-parser to v22 ([#2470](yargs/yargs#2470)) ([639130d](yargs/yargs@639130d)) - exit after async handler done ([#2313](yargs/yargs#2313)) ([e326cde](yargs/yargs@e326cde)) - handle spaces in bash completion ([#2452](yargs/yargs#2452)) ([83b7788](yargs/yargs@83b7788)) - parser-configuration should work well with generated completion script ([#2332](yargs/yargs#2332)) ([888db19](yargs/yargs@888db19)) - propagate Dictionary including undefined in value type ([#2393](yargs/yargs#2393)) ([2b2f7f5](yargs/yargs@2b2f7f5)) - **zsh:** completion no longer requires double tab when using autoloaded ([0dd8fe4](yargs/yargs@0dd8fe4)) ##### Code Refactoring - command names are not derived from modules passed to `command`. ([d90af45](yargs/yargs@d90af45)) - singleton usage of yargs yargs.foo, yargs().argv, has been removed. ([d90af45](yargs/yargs@d90af45)) ##### Build System - minimum node.js versions now `^20.19.0 || ^22.12.0 || >=23`. ([d90af45](yargs/yargs@d90af45))
renovate bot
added a commit
to andrei-picus-tink/auto-renovate
that referenced
this pull request
Jun 14, 2025
| datasource | package | from | to | | ---------- | ------- | ------ | ------ | | npm | yargs | 17.7.2 | 18.0.0 | ## [v18.0.0](https://github.com/yargs/yargs/blob/HEAD/CHANGELOG.md#1800-2025-05-26) ##### ⚠ BREAKING CHANGES - command names are not derived from modules passed to `command`. - singleton usage of yargs yargs.foo, yargs().argv, has been removed. - minimum node.js versions now `^20.19.0 || ^22.12.0 || >=23`. - yargs is now ESM first ##### Features - commandDir now works with ESM files ([#2461](yargs/yargs#2461)) ([27eec18](yargs/yargs@27eec18)) - **locale:** adds hebrew translation ([#2357](yargs/yargs#2357)) ([4266485](yargs/yargs@4266485)) - yargs is now ESM first ([d90af45](yargs/yargs@d90af45)) - **zsh:** Add default completion as fallback ([#2331](yargs/yargs#2331)) ([e02c91b](yargs/yargs@e02c91b)) ##### Bug Fixes - addDirectory do not support absolute command dir ([#2465](yargs/yargs#2465)) ([3a40a78](yargs/yargs@3a40a78)) - allows ESM modules commands to be extensible using visit option ([#2468](yargs/yargs#2468)) ([200e1aa](yargs/yargs@200e1aa)) - **browser:** fix shims so that yargs continues working in browser context ([#2457](yargs/yargs#2457)) ([4ae5f57](yargs/yargs@4ae5f57)) - **build:** address problems with typescript compilation ([#2445](yargs/yargs#2445)) ([8d72fb3](yargs/yargs@8d72fb3)) - coerce should play well with parser configuration ([#2308](yargs/yargs#2308)) ([8343c66](yargs/yargs@8343c66)) - **deps:** update dependency yargs-parser to v22 ([#2470](yargs/yargs#2470)) ([639130d](yargs/yargs@639130d)) - exit after async handler done ([#2313](yargs/yargs#2313)) ([e326cde](yargs/yargs@e326cde)) - handle spaces in bash completion ([#2452](yargs/yargs#2452)) ([83b7788](yargs/yargs@83b7788)) - parser-configuration should work well with generated completion script ([#2332](yargs/yargs#2332)) ([888db19](yargs/yargs@888db19)) - propagate Dictionary including undefined in value type ([#2393](yargs/yargs#2393)) ([2b2f7f5](yargs/yargs@2b2f7f5)) - **zsh:** completion no longer requires double tab when using autoloaded ([0dd8fe4](yargs/yargs@0dd8fe4)) ##### Code Refactoring - command names are not derived from modules passed to `command`. ([d90af45](yargs/yargs@d90af45)) - singleton usage of yargs yargs.foo, yargs().argv, has been removed. ([d90af45](yargs/yargs@d90af45)) ##### Build System - minimum node.js versions now `^20.19.0 || ^22.12.0 || >=23`. ([d90af45](yargs/yargs@d90af45))
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refs to #2467
The test can catch the issue, and pass after fixing, and also pass my test repo.