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

Export InvalidOptionArgumentError in esm #1756

Merged
merged 3 commits into from Jun 20, 2022
Merged

Conversation

everett1992
Copy link
Contributor

After upgrading a typescript project to native es modules imports for
InvalidOptionArgumentError were undefined at runtime but
defined in typescript as an alias to InvalidArgumentError.

# test.ts
import { InvalidOptionArgumentError } from 'commander'
//       ^ No type error here, typescript thinks InvalidOptionArgumentError should exist

assert(InvalidOptionArgumentError != null); // Yet will throw an error

A quick test showed that there was a difference between the commonjs and
ecmascript module. The issue wasn't limited to typescript.

# test.cjs
const { InvalidOptionArgumentError } = require('commander')
assert(InvalidOptionArgumentError != null);
# test.mjs
import { InvalidOptionArgumentError } from 'commander'
assert(InvalidOptionArgumentError != null); // This will throw an error

This commit fixes the issue by exporting InvalidOptionArgumentError from
the esm entry point. Alternatively we could remove
InvalidOptionArgumentError from typings, I'm happy as long as build and
runtime match.

I upgraded a typescript project to native es modules and imported
`InvalidOptionArgumentError` was undefined at runtime, tho typescript
defined it as an alias to InvalidArgumentError.

```
import { InvalidOptionArgumentError } from 'commander'
//       ^ No type error here, typescript thinks InvalidOptionArgumentError should exist

assert(InvalidOptionArgumentError != null); // But it doesn't, this will throw an error
```

A quick test showed that there was a difference between the commonjs and
ecmascript module. The issue wasn't limited to typescript.

```
const { InvalidOptionArgumentError } = require('commander')
assert(InvalidOptionArgumentError != null);
```

```
import { InvalidOptionArgumentError } from 'commander'
assert(InvalidOptionArgumentError != null); // This will throw an error
```

This commit fixes the issue by exporting InvalidOptionArgumentError from
the esm entry point. Alternatively we could remove
InvalidOptionArgumentError from typings, I'm happy as long as build and
runtime match.
@shadowspawn
Copy link
Collaborator

The esm export got dropped/renamed in #1508 when InvalidOptionArgumentError got deprecated. I might have thought it wouldn't matter in the "new" esm code, but as you discovered it is an issue when switching from cjs to esm runtime.

esm.mjs Outdated Show resolved Hide resolved
Co-authored-by: John Gee <john@ruru.gen.nz>
@shadowspawn
Copy link
Collaborator

Looks like there was an indentation boobytrap in the suggestion I made, sorry about that!

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 (just an indentation issue).

@shadowspawn
Copy link
Collaborator

(Pushed indentation fix.)

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.

👍

@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Jun 20, 2022
@shadowspawn shadowspawn merged commit 3ae30a2 into tj:develop Jun 20, 2022
@shadowspawn
Copy link
Collaborator

Thanks @everett1992

@shadowspawn
Copy link
Collaborator

Released in Commander v9.4.0

@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Jul 15, 2022
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.

None yet

3 participants