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: cannot find package.json on ESM #2123

Merged
merged 3 commits into from May 14, 2022

Conversation

jopemachine
Copy link
Contributor

@jopemachine jopemachine commented Jan 27, 2022

Fixes #2122.

In the case of ava, mainFilename changes like below way.

// existing code's value:
mainFilename: /usr/local/lib/

// this PR's value:
mainFilename: /usr/local/lib/node_modules/ava/

This enable yargs to find ava's package.json properly.

@jly36963 jly36963 requested a review from bcoe Mar 26, 2022
@bcoe
Copy link
Member

@bcoe bcoe commented Mar 28, 2022

@jopemachine I don't fully understand this fix, is the issue that there are multiple instances of node_modules in the path?

@jopemachine
Copy link
Contributor Author

@jopemachine jopemachine commented Mar 29, 2022

I don't fully understand this fix, is the issue that there are multiple instances of node_modules in the path?

Yes, as I mentioned in this comment, node_modules appears more than once in the path (__dirname).

So, existing code can't find proper mainFilename, package.json, that's why globally installed this module's version command prints unknown.

@jamescdavis
Copy link

@jamescdavis jamescdavis commented May 10, 2022

@bcoe I tested this fix and I can confirm that it fixes the issue. Seems like a good solution. Hopefully this can be merged and released soon. 🙂

bcoe
bcoe approved these changes May 14, 2022
@bcoe bcoe merged commit e0823dd into yargs:main May 14, 2022
7 checks passed
@bcoe
Copy link
Member

@bcoe bcoe commented May 14, 2022

@jopemachine nit, it would be nice to have a test for this, but going ahead and merging with the hopes of unblocking issues with ava.

apologies for the slow review.

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.

4 participants