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

feat: fall back to global npm and yarn if not found #871

Merged
merged 8 commits into from
May 15, 2024

Conversation

mdonnalley
Copy link
Contributor

@mdonnalley mdonnalley commented May 13, 2024

  • Default to globally installed npm and yarn if they can't be found in plugin-plugin's node_modules
  • Use spawn instead of fork

Fixes #864
@W-15703207@

@mdonnalley mdonnalley force-pushed the mdonnalley/lite-version branch from f41be01 to e69f75a Compare May 13, 2024 20:58
@mdonnalley mdonnalley force-pushed the mdonnalley/lite-version branch from 50f794d to bafa6c1 Compare May 14, 2024 17:58
@@ -42,9 +42,12 @@ jobs:
matrix:
os: [ubuntu-latest, windows-latest]
test: ['test:integration:install', 'test:integration:link']
lite: [true, false]
Copy link
Contributor

Choose a reason for hiding this comment

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

That's clever

return require.resolve('yarn/bin/yarn.js', {paths: [this.config.root, fileURLToPath(import.meta.url)]})
if (this.bin) return this.bin
try {
this.bin = require.resolve('yarn/bin/yarn.js', {paths: [this.config.root, fileURLToPath(import.meta.url)]})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the different resolve logic for yarn? When I remove yarn from my linked plugin-plugins it resolves the to yarn that is included with the sf cli rather than falling back to my locally installed version
@oclif/plugin-plugins:yarn yarn binary path /Users/ewillhoit/.nvm/versions/node/v20.12.2/lib/node_modules/@salesforce/cli/node_modules/yarn/bin/yarn.j

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pulled the logic in exactly as it was in the last major: https://github.com/oclif/plugin-plugins/blob/4.3.10/src/yarn.ts#L30

I can replicate the same logic that we use for npm... but I'm wondering if it's worth it if we one day hope to remove the "install dependencies after linking" logic altogether. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's fine since we'd like to get rid of this. I don't really see an issue with this, I was just curious if it was deliberate.

@iowillhoit
Copy link
Contributor

QA NOTES

  • 🟢 Able to install a plugin with bundled npm
  • 🟢 Able to uninstall a plugin with bundled npm
  • 🟢 Able to install a plugin with local (nvm) version of npm
  • 🟢 Able to uninstall a plugin with local (nvm) version of npm
  • 🟢 Able to link a plugin with bundled yarn
  • 🟢 Able to unlink a plugin with bundled yarn
  • 🟢 Able to link a plugin with local version of yarn
  • 🟢 Able to unlink a plugin with local version of yarn

@mdonnalley mdonnalley changed the title feat: publish lite version feat: fall back to global npm and yarn if not found May 14, 2024
@mdonnalley mdonnalley merged commit 48dc584 into main May 15, 2024
17 checks passed
@mdonnalley mdonnalley deleted the mdonnalley/lite-version branch May 15, 2024 15:46
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.

Would it be possible to use the system's npm?
3 participants