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 factory functions with typescript whole module import #2013

Merged

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Sep 11, 2023

Pull Request

A subtle problem with Commander exports got reported in #1974

Problem

The factory functions like createOption were not available using a TypeScript whole module import (import * as commander) when esModuleInterop: true.

Solution

Explicitly export the factory functions, rather than just rely on implicitly being available via global program. This is also consistent with eventual goal of dropping deprecated default export of global program.

Inspired by #1974. Lists @aweebit as co-author in commit comment.

## ChangeLog
  • fix: global factory functions now available when using TypeScript whole module import (import * as commander)

NB: re-export reverted in #2014 due to problems!

Co-authored-by: Wee Bit <aweebit64@gmail.com>
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.

LGTM

@shadowspawn shadowspawn merged commit 384f17b into tj:develop Sep 12, 2023
11 checks passed
@shadowspawn shadowspawn deleted the feature/fix-typescript-whole-module-import branch September 12, 2023 07:12
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Sep 12, 2023
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Sep 12, 2023

I did think it looked like a circular reference, but if it made TypeScript happy...

However, a little amusingly and annoyingly, TypeScript thinks it is circular too! (Apparently I didn't try the full tests before now.)

It was interesting learning about the issue and I am still happy the tests are improved, but planning to comment out the factory functions again. There is an easy work-around if anyone hits the issue, which is to import the factory function using a named import.

% npm run typescript-checkJS

> commander@11.0.0 typescript-checkJS
> tsc --allowJS --checkJS index.js lib/*.js --noEmit

index.js:37:1 - error TS2303: Circular definition of import alias 'createCommand'.

37 exports.createCommand = program.createCommand;
   ~~~~~~~~~~~~~~~~~~~~~

index.js:38:1 - error TS2303: Circular definition of import alias 'createArgument'.

38 exports.createArgument = program.createArgument;
   ~~~~~~~~~~~~~~~~~~~~~~

index.js:39:1 - error TS2303: Circular definition of import alias 'createOption'.

39 exports.createOption = program.createOption;
   ~~~~~~~~~~~~~~~~~~~~


Found 3 errors in the same file, starting at: index.js:37

@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Feb 3, 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 this pull request may close these issues.

2 participants