-
Notifications
You must be signed in to change notification settings - Fork 36
Re: Fix plugins not being found when no node_modules exists #289
Re: Fix plugins not being found when no node_modules exists #289
Conversation
This test should pass when it has been updated to use resolve
Assume resolve returns an entry file not a directory
Thanks for the contribution! Before we can merge this, we need @benatshippabo to sign the Salesforce.com Contributor License Agreement. |
Codecov Report
@@ Coverage Diff @@
## master #289 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 2 2
Lines 12 12
Branches 3 3
======================================
Misses 12 12 Continue to review full report at Codecov.
|
@benatshippabo I tested this locally with sfdx-cli and I'm still seeing the same issue:
|
Thanks for the prompt feedback @mdonnalley! I'm not sure how to test this against that repository, edit: edit 2: |
@benatshippabo I'm still seeing the same error with your latest changes. You can test against sfdx-cli like this:
Just make sure to compile your changes before running |
Thanks for the test steps. I just figured out the issue! The issue is that the # at the root directory of sfdx
yarn
node -e "console.log(require.resolve('@oclif/parser'))"
node -e "console.log(require.resolve('@oclif/plugin-commands'))" If we inspect each of those package's We can test the fix by adding # we can see here that it will now resolve properly 😃
node -e "console.log(require.resolve('@oclif/plugin-commands'))" I think this will require some minor updates to all the missing plugins so that is is compliant with the node module resolution algorithm. List of plugins missing the `main` field in `package.json`
This will probably require some effort to update these plugins. Thoughts @mdonnalley? |
@benatshippabo Good find! Unfortunately, if we require that plugins must specify the Is it possible to fallback to the old behavior whenever a module isn't found? |
} | ||
yield from | ||
} | ||
async function findRootLegacy(name: string | undefined, root: string): Promise<string | undefined> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benatshippabo Good find!
Unfortunately, if we require that plugins must specify the
main
field then this would be a breaking change, which we're not willing to do on this package since it will soon be in maintenance mode.
That's unfortunate, maybe we can see these changes once the package that will supersede this one is released?
Is it possible to fallback to the old behavior whenever a module isn't found?
Done! @mdonnalley I re-added the legacy dependency resolution function as a fallback
Thanks @benatshippabo Testing is going well so far. I'm going to test it out a bit more before merging though oclif/core is the heir apparent to this project so it'd be great if you could port these changes over there as well. It should be a fairly straightforward copy and paste since most of this code is unchanged: https://github.com/oclif/core/blob/main/src/config/plugin.ts#L41 I would prefer to keep the legacy fallback to persevere backwards compatibility between plugins that use oclif/config and plugins that use oclif/core. However, I will add a note about adding the |
@benatshippabo - thanks so much for your thorough and thoughtful work on this PR. This comment is amazing. 🙌 |
Awesome! Thanks for being so responsive for this pull request.
Done oclif/core#309 |
Hi @mdonnalley Was there a release cut for this merge? If not - do you know when we can expect a version to come out with these changes? Thanks! |
@mdonnalley bump on the above ^^ any way we can get a tag version to use with this change? Thanks! |
The latest version |
Description
Technical Details
Uses native
require.resolve
to resolve dependency package's path instead of the third partyresolve
package. Addresses @mdonnalley's comment #171 (comment)For Yarn 2+ zero install users who needed this issue resolved
yarn patch
to addmain: lib/index.js
inside thepackage.json
of oclif plugins that still fail to resolveContext
#171