Fix plugins not being found when no node_modules exists #171

TPHRyan · 2020-08-30T14:28:04Z

Fixes #170.

As I mentioned in the issue, Yarn 2 uses a different package system that does not have node_modules. As such, relying on node_modules should be discouraged.

Yarn suggests using the resolve package, and this is the approach I have taken.

Please let me know if any further changes are required. I tried to conform to idioms of the code, but for testing in particular I wasn't sure what the guidelines were.

This test should pass when it has been updated to use resolve

Assume resolve returns an entry file not a directory

salesforce-cla · 2020-08-30T14:28:08Z

Thanks for the contribution! Before we can merge this, we need @TPHRyan to sign the Salesforce.com Contributor License Agreement.

codecov · 2020-08-30T14:29:27Z

Codecov Report

Merging #171 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #171   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           2       2           
  Lines          12      12           
  Branches        3       3           
======================================
  Misses         12      12

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07a0670...7925d35. Read the comment docs.

tobilen · 2021-03-14T14:45:58Z

@jdxcode anything missing here? this issue is preventing apollo tooling from running under yarn 2

AaronBuxbaum · 2021-04-08T16:37:45Z

Any movement on this PR? It's been a long while, and this would help quite a few packages that depend on it to move to Yarn 2.

amphro · 2021-04-12T17:05:51Z

I don't see any problem with this, but I want to try it out. @AaronBuxbaum have you also been able to try this PR to confirm that it works and doesn't break any existing functionality?

I hope you have also seen the announcement about oclif/core. I'm assuming that library will also have that problem? @TPHRyan could we also do a PR there as well?

AaronBuxbaum · 2021-04-12T17:09:28Z

I don't see any problem with this, but I want to try it out. @AaronBuxbaum have you also been able to try this PR to confirm that it works and doesn't break any existing functionality?

Yes, I'm using it now and as far as I'm concerned there are no issues at all with this. It's a dependency of a different library, so it's a limited test case, but it looks solid to me.

amphro

When I link this with https://github.com/salesforcecli/sfdx-cli/ it blows up. Passing in basedir as mentioned in the comments fixes some errors, but it still doesn't resolve everything. I don't have the time right now to try to see why it is still failing. Unless we can see why resolve isn't finding the packages, we may have to add in the old code as a backup if resolve can't find it.

amphro · 2021-04-12T20:22:04Z

src/plugin.ts

+  if (name) {
+    let pkgPath
+    try {
+      pkgPath = await resolvePackage(name)


I think you need to pass in { basedir: root } because it may need to look outside of the current path. i.e. where user plugins are stored.

With that said, the resolve library isn't working how I thought it would. If I add the basedir above, it will still sometimes throw MODULE_NOT_FOUND even though it seems to be in the resolution path.

For example, I modified resolvePackage to take in the same options object that resolve takes and pass that to resolve.

// name = '@salesforce/plugin-evergreen-build' // root = '/Users/t.dvornik/.local/share/sfdx/node_modules/@salesforce/plugin-evergreen' ... // The root may not always be a dir, so ensure it is before passing it into resolve if (path.extname(root)) { root = path.dirname(root) } try { pkgPath = await util_2.resolvePackage(name, { basedir: root }); } ...

That package is in /Users/t.dvornik/.local/share/sfdx/node_modules/@salesforce/plugin-evergreen-build which should be in node module resolution but resolve isn't resolving it.

Thanks for looking at this - this issue flew under the radar for me for a minute, but I'll have a look to see if I can fix it sometime soon. It's not high priority for me but evidently there's a few others others also waiting on a fix.

Any reason we don't just use the native require.resolve over the resolve package? it doesn't appear that we really need it to resolve asynchronously for our use case.

AaronBuxbaum · 2021-07-19T20:12:04Z

Any progress on this? @TPHRyan

I'm hoping to use workspaces in my Yarn 2 repo, but that throws errors from @oclif/config even with the existing patch that previously worked:

module: @oclif/config@1.17.0
task: runHook init
plugin: @percy/cli
root: /Users/aaron/Work/web/.yarn/cache/@percy-cli-npm-1.0.0-beta.59-cc1949c1e6-3ebc2fb960.zip/node_modules/@percy/cli
See more details with DEBUG=*
Error: can't resolve reference /snapshot#/properties/name from id /config/static
    at Object.code (/Users/aaron/Work/web/.yarn/cache/ajv-npm-8.6.2-712407c2bc-9969d6c0e6.zip/node_modules/ajv/dist/vocabularies/core/ref.js:21:19)
    at keywordCode (/Users/aaron/Work/web/.yarn/cache/ajv-npm-8.6.2-712407c2bc-9969d6c0e6.zip/node_modules/ajv/dist/compile/validate/index.js:454:13)
    at /Users/aaron/Work/web/.yarn/cache/ajv-npm-8.6.2-712407c2bc-9969d6c0e6.zip/node_modules/ajv/dist/compile/validate/index.js:185:25
    at CodeGen.code (/Users/aaron/Work/web/.yarn/cache/ajv-npm-8.6.2-712407c2bc-9969d6c0e6.zip/node_modules/ajv/dist/compile/codegen/index.js:439:13)
    at CodeGen.block (/Users/aaron/Work/web/.yarn/cache/ajv-npm-8.6.2-712407c2bc-9969d6c0e6.zip/node_modules/ajv/dist/compile/codegen/index.js:568:18)
    at schemaKeywords (/Users/aaron/Work/web/.yarn/cache/ajv-npm-8.6.2-712407c2bc-9969d6c0e6.zip/node_modules/ajv/dist/compile/validate/index.js:185:13)
    at typeAndKeywords (/Users/aaron/Work/web/.yarn/cache/ajv-npm-8.6.2-712407c2bc-9969d6c0e6.zip/node_modules/ajv/dist/compile/validate/index.js:128:5)
    at subSchemaObjCode (/Users/aaron/Work/web/.yarn/cache/ajv-npm-8.6.2-712407c2bc-9969d6c0e6.zip/node_modules/ajv/dist/compile/validate/index.js:115:5)
    at subschemaCode (/Users/aaron/Work/web/.yarn/cache/ajv-npm-8.6.2-712407c2bc-9969d6c0e6.zip/node_modules/ajv/dist/compile/validate/index.js:91:13)
    at KeywordCxt.subschema (/Users/aaron/Work/web/.yarn/cache/ajv-npm-8.6.2-712407c2bc-9969d6c0e6.zip/node_modules/ajv/dist/compile/validate/index.js:428:9)

this is effectively the same setup as previously worked, except with a workspace that now owns the percy and oclif packages, instead of directly on root

QWp6t · 2021-07-20T00:03:50Z

As a workaround, you can tell Yarn 2 to use node-modules instead of pnp.

.yarnrc.yml

nodeLinker: node-modules

Reference: https://yarnpkg.com/configuration/yarnrc/#nodeLinker

AaronBuxbaum · 2021-07-20T01:22:15Z

nodeLinker: node-modules

For me at least, that doesn't do it, though it could be that it's not actually oclif here, I haven't dug into it

(node:17050) Error Plugin: @percy/cli: can't resolve reference /config/snapshot#/properties/widths from id /config/static
module: @oclif/config@1.17.0
task: runHook init
plugin: @percy/cli
root: /Users/aaron/Work/web/node_modules/@percy/cli
See more details with DEBUG=*
    Error: can't resolve reference /config/snapshot#/properties/widths 
    from id /config/static

It's also not a greaaat solution even if it did work because the node_modules linker defeats a lot of the purpose of Yarn 2

timini · 2021-09-03T15:29:49Z

can we get this merged already??

dangreenisrael · 2021-11-29T17:44:10Z

@peternhale. Would you mind taking a look at this? It seems to be blocking a lot of people. 🙏

mdonnalley · 2021-11-29T20:35:17Z

@TPHRyan Sorry for the delays on this PR!

I'll merge this PR once we're done testing on our end. I'd strongly suggest that you open a PR with the same changes against oclif/core since this project will be going into maintenance mode in the next 1-2 weeks once core is officially released

This reverts commit e5739c4.

mdonnalley · 2021-11-29T23:29:53Z

@TPHRyan Despite our internal testing, we encountered a bug with this change in the sfdx cli after it was published: forcedotcom/cli#1300

I decided to revert the commit for now. I'm happy to merge your changes again once you come up with a fix

benatshippabo · 2021-11-30T01:55:28Z

I decided to revert the commit for now. I'm happy to merge your changes again once you come up with a fix

@mdonnalley I have forked and updated this pr in #289 let me know if that fixes the issues

TPHRyan added 3 commits August 30, 2020 23:10

Implement test for plugin load

ee785b2

This test should pass when it has been updated to use resolve

Update findRoot to use resolve function

a067b51

Fix findRoot not working due to incorrect assumptions

7925d35

Assume resolve returns an entry file not a directory

salesforce-cla bot added the cla:missing label Aug 30, 2020

salesforce-cla bot added cla:signed and removed cla:missing labels Aug 30, 2020

dc-pm mentioned this pull request Feb 9, 2021

Yarn V2 compatibility apollographql/apollo-tooling#1850

Open

wwilsman mentioned this pull request Apr 8, 2021

Does not work with Yarn 2 Plug'n'Play (Berry) percy/cli#295

Closed

amphro suggested changes Apr 12, 2021

View reviewed changes

techpeace mentioned this pull request Jun 28, 2021

Commands must be compiled to run during development voxable/cli#5

Closed

peternhale requested a review from mdonnalley November 29, 2021 18:40

mdonnalley merged commit e5739c4 into oclif:master Nov 29, 2021

mdonnalley added a commit that referenced this pull request Nov 29, 2021

Revert "Fix plugins not being found when no node_modules exists (#171)"

3ac747b

This reverts commit e5739c4.

benatshippabo mentioned this pull request Nov 30, 2021

Re: Fix plugins not being found when no node_modules exists #289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix plugins not being found when no node_modules exists #171

Fix plugins not being found when no node_modules exists #171

TPHRyan commented Aug 30, 2020

salesforce-cla bot commented Aug 30, 2020

codecov bot commented Aug 30, 2020 •

edited

tobilen commented Mar 14, 2021

AaronBuxbaum commented Apr 8, 2021

amphro commented Apr 12, 2021

AaronBuxbaum commented Apr 12, 2021

amphro left a comment •

edited

amphro Apr 12, 2021

TPHRyan May 18, 2021

benatshippabo Oct 28, 2021

AaronBuxbaum commented Jul 19, 2021

QWp6t commented Jul 20, 2021

AaronBuxbaum commented Jul 20, 2021

timini commented Sep 3, 2021

dangreenisrael commented Nov 29, 2021

mdonnalley commented Nov 29, 2021

mdonnalley commented Nov 29, 2021

benatshippabo commented Nov 30, 2021

Fix plugins not being found when no node_modules exists #171

Fix plugins not being found when no node_modules exists #171

Conversation

TPHRyan commented Aug 30, 2020

salesforce-cla bot commented Aug 30, 2020

codecov bot commented Aug 30, 2020 • edited

Codecov Report

tobilen commented Mar 14, 2021

AaronBuxbaum commented Apr 8, 2021

amphro commented Apr 12, 2021

AaronBuxbaum commented Apr 12, 2021

amphro left a comment • edited

Choose a reason for hiding this comment

amphro Apr 12, 2021

Choose a reason for hiding this comment

TPHRyan May 18, 2021

Choose a reason for hiding this comment

benatshippabo Oct 28, 2021

Choose a reason for hiding this comment

AaronBuxbaum commented Jul 19, 2021

QWp6t commented Jul 20, 2021

AaronBuxbaum commented Jul 20, 2021

timini commented Sep 3, 2021

dangreenisrael commented Nov 29, 2021

mdonnalley commented Nov 29, 2021

mdonnalley commented Nov 29, 2021

benatshippabo commented Nov 30, 2021

codecov bot commented Aug 30, 2020 •

edited

amphro left a comment •

edited