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 Yarn detection when using workspaces, fix .yarnrc.yml loading #1148

Merged
merged 8 commits into from
Jun 30, 2022

Conversation

srmagura
Copy link
Contributor

@srmagura srmagura commented Jun 25, 2022

Issues Addressed

  1. Resolves npm erroneously used in Yarn Workspaces monorepo #1120.
  2. Resolves an issue I noticed during implementation: if the packageFile option was passed, the code still looked in . (current directory) when checking for a yarn.lock. Now it starts looking in the directory of the package file. I believe this is the correct behavior.
  3. Resolves the local .yarnrc.yml always being loaded from ./.yarnrc.yml, which would not work if the packageFile option is passed or we are in a non-root workspace directory.
  4. Resolves the user .yarnrc.yml never being loaded. The code used fs.existsSync('~/.yarnrc.yml'), but this does not interpret ~ as the user's home directory.

Notes

  • I put the new test file in the same directory as the file being tested. This is my preferred pattern (rationale here), but I know this differs from the organization used for the other test files in the repo. I'm happy to move the test file if you tell me where it should go.
  • I added a mocha script to enable running a single test file, e.g. npm run mocha -- src/lib/determinePackageManager.test.ts
  • If you'd like to test fixes 1 and 2 against a "real" repository that uses Yarn workspaces, you can clone https://github.com/srmagura/npm-check-updates-1120-test. Fixes 3 and 4 are easiest to test if you have access to a private npm registry, e.g. the registry for FontAwesome Pro.
  • I tested .npmrc loading with npm workspaces and it worked, so that doesn't need to be fixed. 🎉

@srmagura srmagura marked this pull request as draft June 25, 2022 19:05
@srmagura
Copy link
Contributor Author

Converting to draft because, I realized the logic for finding the .npmrc/.yarnrc.yml is probably also broken...

@srmagura srmagura changed the title Add determinePackageManager, fix Yarn detection when using workspaces Fix Yarn detection when using workspaces, fix .yarnrc.yml loading Jun 26, 2022
@srmagura srmagura marked this pull request as ready for review June 26, 2022 13:24
@raineorshine
Copy link
Owner

  • I put the new test file in the same directory as the file being tested. This is my preferred pattern (rationale here), but I know this differs from the organization used for the other test files in the repo. I'm happy to move the test file if you tell me where it should go.

I appreciate the point, though let's stick with a separate test folder to be consistent with the project's conventions.

Copy link
Owner

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

package.json Outdated Show resolved Hide resolved
src/lib/findLockAndConfigFiles.ts Outdated Show resolved Hide resolved
src/lib/initOptions.ts Show resolved Hide resolved
@srmagura
Copy link
Contributor Author

Thanks for the feedback, should be good now :)

@raineorshine raineorshine merged commit e445722 into raineorshine:main Jun 30, 2022
@srmagura srmagura deleted the fix-1120 branch June 30, 2022 13:04
@srmagura
Copy link
Contributor Author

Thanks for merging. I checked out your changes, it all looked good except, I find nested ternaries to be hard to read. I guess not everyone feels that way though. No need to change 👍

@raineorshine
Copy link
Owner

Thanks for your contribution! I'll release it shortly.

Thanks for merging. I checked out your changes, it all looked good except, I find nested ternaries to be hard to read. I guess not everyone feels that way though. No need to change 👍

👏👏👏 preferences 🙃🙃🙃

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.

npm erroneously used in Yarn Workspaces monorepo
2 participants