-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 aggregated import and remove test for unsupported default export in TypeScript #1974
Conversation
In the test file, an aggregating import (import * as commander) was used to allegedly import the default export. That is not the same as actually importing the default export (import commander). Fixing the import revealed the following error: TS1192: Module '".../commander.js/typings/index"' has no default export. The reason is that the default export is not publicly declared because it is deprecated, and so should not be used. The test was therefore unnecessary.
The aggregated import (import * as commander) doesn't deliver all expected functions when "esModuleInterop": true is set in TSConfig, including when set implicitly because "module" is set to "node16" or "nodenext", like is now the case in our TSConfig after the last commit. The new test block covers that.
The TypeScript aggregated/namespace const commander = require('commander');
function test(x) {
console.log(`${x}: ${Object.prototype.hasOwnProperty.call(commander, x)}`);
}
test('Argument');
test('createCommand');
test('program');
test('xyz');
commander.createCommand().parse(['--help'], { from: 'user' }); % node r.js
Argument: true
createCommand: false
program: true
xyz: false
Usage: program [options]
Options:
-h, --help display help for command |
Doing explicit exports for the exported properties fixes the issue, well worked out. I don't think it makes sense to test it because it is sensitive to the TypeScript settings which we choose for other reasons. So just change This resolves the uncertainties mentioned in #1969. |
There was an error caused by circular references or something like that before, but I forgot under what circumstances and cannot reproduce it now. Removed the directives in d4a83fb.
I think it should be tested with different settings then. My suggestion is to create two identical files just for the aggregated import tests and to transform them with different |
tests/ts-imports.test.ts
Outdated
import * as commander from '../'; | ||
|
||
const { program, Command, Option, CommanderError, InvalidArgumentError, InvalidOptionArgumentError, Help, createCommand } = commander; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was previously testing two types of import. I still want to test both.
However, the aggregated import can test everything (especially the create routines!) rather than the now deprecated global object which was what I was worried about at the time.
Thinking about it more I do want to test both aggregated and deconstructed imports, since they do vary so unexpectedly in TypeScript, and can run same tests over both. But I can't be bothered with actually setting up custom tsconfig with different configuration. Not even for just two of the many permutations that change the import implementation. I'll add notes in the PR where wrenching the config: #1969 |
Current summary:
|
I have now added the normal import, but I am not sure the aggregated one is guaranteed to always behave the same when it is used together with the normal one and when used without it. Might be a better idea to, again, test them in separate files, like |
@@ -11,38 +11,54 @@ function checkClass(obj: object, name: string): void { | |||
expect(obj.constructor.name).toEqual(name); | |||
} | |||
|
|||
test('legacy default export of global Command', () => { | |||
checkClass(commander, 'Command'); | |||
describe('commander', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
describe('commander', () => { | |
describe('import * as commander', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Or you were calling it aggregated import
which would be ok too.)
test('program', () => { | ||
checkClass(program, 'Command'); | ||
}); | ||
describe('class name', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
describe('class name', () => { | |
describe('named imports', () => { |
That would be fine. (I checked the test does fail with double import when |
Please have a look at the commit descriptions for details.
ChangeLog
Fixed
createCommand
,createArgument
,createOption
exports to TypeScript aggregated ESM import (import * as commander
) when"esModuleInterop": true
is set in TSConfigPeer PRs
Changes are to be merged into…