feat: better support yarn PnP #839

mdonnalley · 2023-10-19T19:38:17Z

Add ability to find plugin root for PnP plugins that lack a main or export defined in their package.json.
Put all findRoot related functionality into its own file for cleanliness purposes

Huge thanks to @danieltroger for figuring out the implementation for this!

Fixes #806

@W-14331357@

mdonnalley · 2023-10-19T21:04:15Z

@danieltroger I published a prerelease for you test these changes out on your project, 3.4.1-dev.0 - let me know if it's working for you or if any other changes need to be made. Thanks!

danieltroger · 2023-10-20T07:46:59Z

src/util/find-root.ts

+  // Iterate on each workspace
+  for (const locator of pnp.getDependencyTreeRoots()) {
+    const foundSomething = traverseDependencyTree(locator)
+    if (foundSomething) return foundSomething
+  }


This is what I referenced with

If there are multiple versions of the plugin that's trying to be found, the wrong one might be selected (root is ignored)

In #806

And I'm not sure if the issue is fixed?

Suppose that someone has two workspaces and they each contain a different version of the oclif plugin, this wouldn't necessarily take the oclif plugin in the correct workspace, but rather the first one found. This might lead to frustrating issues where someone updates their plugin version (in the workspace they have that oclif version installed in) but it doesn't do anything because oclif picks a workspace that has another version.

@danieltroger This might be a scenario where I prefer to wait until someone actually has this issue. Also I see this code path as a fallback for when people do not specify a main or exports, which is something we strongly advise against because it's much more performant to do a require.resolve than it is to traverse the dependency tree for a matching plugin.

So if someone were to have such an issue, I would first suggest that they define main or exports in their package.json.

danieltroger · 2023-10-20T08:09:14Z

@mdonnalley Thanks a lot for following up on my issue report and working on support for yarn PnP!

I removed my patch and tried to get the new version by running yarn set resolution @oclif/core@npm:2.11.7 3.4.1-dev.0

And now my @oclif/core situation looks like this

daniel@mmmmmmmmmm shopify % yarn why @oclif/core                                   
├─ @oclif/plugin-commands@npm:2.2.21
│  └─ @oclif/core@npm:2.15.0 (via npm:^2.11.1)
│
├─ @oclif/plugin-help@npm:5.2.15
│  └─ @oclif/core@npm:2.15.0 (via npm:^2.11.1)
│
├─ @oclif/plugin-plugins@npm:3.1.8
│  └─ @oclif/core@npm:2.15.0 (via npm:^2.8.7)
│
├─ @shopify/app@npm:3.49.5
│  └─ @oclif/core@npm:3.4.1-dev.0 (via npm:2.11.7)
│
├─ @shopify/app@npm:3.49.5 [f6877]
│  └─ @oclif/core@npm:3.4.1-dev.0 (via npm:2.11.7)
│
├─ @shopify/cli-kit@npm:3.49.5
│  └─ @oclif/core@npm:3.4.1-dev.0 (via npm:2.11.7)
│
├─ @shopify/cli@npm:3.49.5
│  └─ @oclif/core@npm:3.4.1-dev.0 (via npm:2.11.7)
│
├─ @shopify/plugin-cloudflare@npm:3.49.5
│  └─ @oclif/core@npm:3.4.1-dev.0 (via npm:2.11.7)
│
├─ @shopify/plugin-did-you-mean@npm:3.49.5
│  └─ @oclif/core@npm:3.4.1-dev.0 (via npm:2.11.7)
│
└─ shopify-app@workspace:.
   └─ @oclif/core@npm:3.4.1-dev.0 (via npm:2.11.7)

But unfortunately I'm getting a new error

╭─ error ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│                                                                                                                                                │
│  config.plugins.map is not a function                                                                                                          │
│                                                                                                                                                │
│  To investigate the issue, examine this stack trace:                                                                                           │
│    at registerCleanBugsnagErrorsFromWithinPlugins                                                                                              │
│    (.yarn/unplugged/@shopify-cli-kit-npm-3.49.5-f269164f4b/node_modules/@shopify/cli-kit/src/public/node/error-handler.ts:161)                 │
│      config.plugins.map(async (plugin) => {                                                                                                    │
│    at init (.yarn/unplugged/@shopify-cli-kit-npm-3.49.5-f269164f4b/node_modules/@shopify/cli-kit/src/public/node/base-command.ts:39)           │
│      await registerCleanBugsnagErrorsFromWithinPlugins(this.config)                                                                            │
│                                                                                                                                                │
╰────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯

Any idea what might be causing this? Something you've seen before? Maybe there were some other changes in oclif that shopify is not on top of?

Here's the diff to my lockfile, just to make sure I've installed the right stuff?

diff --git a/shopify/yarn.lock b/shopify/yarn.lock
index bf54a998a5..9ce2048e10 100644
--- a/shopify/yarn.lock
+++ b/shopify/yarn.lock
@@ -2960,10 +2960,9 @@ __metadata:
   linkType: hard
 
 "@oclif/core@npm:2.11.7":
-  version: 2.11.7
-  resolution: "@oclif/core@npm:2.11.7"
+  version: 3.4.1-dev.0
+  resolution: "@oclif/core@npm:3.4.1-dev.0"
   dependencies:
-    "@types/cli-progress": ^3.11.0
     ansi-escapes: ^4.3.2
     ansi-styles: ^4.3.0
     cardinal: ^2.1.1
@@ -2971,8 +2970,7 @@ __metadata:
     clean-stack: ^3.0.1
     cli-progress: ^3.12.0
     debug: ^4.3.4
-    ejs: ^3.1.8
-    fs-extra: ^9.1.0
+    ejs: ^3.1.9
     get-package-type: ^0.1.0
     globby: ^11.1.0
     hyperlinker: ^1.0.0
@@ -2982,18 +2980,15 @@ __metadata:
     natural-orderby: ^2.0.3
     object-treeify: ^1.1.33
     password-prompt: ^1.1.2
-    semver: ^7.5.3
     slice-ansi: ^4.0.0
     string-width: ^4.2.3
     strip-ansi: ^6.0.1
     supports-color: ^8.1.1
     supports-hyperlinks: ^2.2.0
-    ts-node: ^10.9.1
-    tslib: ^2.5.0
     widest-line: ^3.1.0
     wordwrap: ^1.0.0
     wrap-ansi: ^7.0.0
-  checksum: 12b68c871f896d5eb201624e147da9ac5b7be6911513834d84c03e266a76cfc721bf2d36d859453c80943baaad27f4a520202e06338482eaecb2034afc216ee4
+  checksum: 5277d77e0cc94f0f67c75daf7919038c5e0306381e38242becde7b4ddfd05c1a85feaeeccf04c2aedae1ca392f14fe1a0f3b29933d54ee38070f0b994e7ac274
   languageName: node
   linkType: hard
 
@@ -3033,44 +3028,6 @@ __metadata:
   languageName: node
   linkType: hard
 
-"@oclif/core@patch:@oclif/core@npm:2.11.7#.yarn/patches/@oclif-core-npm-2.11.7-481edfbe66.patch::locator=shopify-app%40workspace%3A.":
-  version: 2.11.7
-  resolution: "@oclif/core@patch:@oclif/core@npm%3A2.11.7#.yarn/patches/@oclif-core-npm-2.11.7-481edfbe66.patch::version=2.11.7&hash=040f2f&locator=shopify-app%40workspace%3A."
-  dependencies:
-    "@types/cli-progress": ^3.11.0
-    ansi-escapes: ^4.3.2
-    ansi-styles: ^4.3.0
-    cardinal: ^2.1.1
-    chalk: ^4.1.2
-    clean-stack: ^3.0.1
-    cli-progress: ^3.12.0
-    debug: ^4.3.4
-    ejs: ^3.1.8
-    fs-extra: ^9.1.0
-    get-package-type: ^0.1.0
-    globby: ^11.1.0
-    hyperlinker: ^1.0.0
-    indent-string: ^4.0.0
-    is-wsl: ^2.2.0
-    js-yaml: ^3.14.1
-    natural-orderby: ^2.0.3
-    object-treeify: ^1.1.33
-    password-prompt: ^1.1.2
-    semver: ^7.5.3
-    slice-ansi: ^4.0.0
-    string-width: ^4.2.3
-    strip-ansi: ^6.0.1
-    supports-color: ^8.1.1
-    supports-hyperlinks: ^2.2.0
-    ts-node: ^10.9.1
-    tslib: ^2.5.0
-    widest-line: ^3.1.0
-    wordwrap: ^1.0.0
-    wrap-ansi: ^7.0.0
-  checksum: 26a27f03e14b27509ed3fd0770c47abc64a7de0858d2f89cd318f3c582c14a8711e2e78985c80815ce714e4d9fc4b4ec6a08202cffe225f8413b6afe2d405a7e
-  languageName: node
-  linkType: hard
-
 "@oclif/plugin-commands@npm:2.2.21":
   version: 2.2.21
   resolution: "@oclif/plugin-commands@npm:2.2.21"
@@ -7793,7 +7750,7 @@ __metadata:
   languageName: node
   linkType: hard
 
-"ejs@npm:^3.1.8":
+"ejs@npm:^3.1.8, ejs@npm:^3.1.9":
   version: 3.1.9
   resolution: "ejs@npm:3.1.9"
   dependencies:
@@ -9016,7 +8973,7 @@ __metadata:
   languageName: node
   linkType: hard
 
-"fs-extra@npm:^9.0, fs-extra@npm:^9.1.0":
+"fs-extra@npm:^9.0":
   version: 9.1.0
   resolution: "fs-extra@npm:9.1.0"
   dependencies:
@@ -14712,8 +14669,6 @@ __metadata:
   dependenciesMeta:
     "@depict-ai/[email protected]":
       unplugged: true
-    [email protected]:
-      unplugged: true
   languageName: unknown
   linkType: soft

danieltroger · 2023-10-20T08:25:31Z

Ok I tracked down the config.plugins.map issue, it comes from this package: https://www.npmjs.com/package/@shopify/cli-kit

And the required change is to change config.plugins to [...config.plugins.values()] since it seems like config.plugins has been changed from being an array to being a map from plugin name to plugin.

Before (crashes)

After (works)

~~Patch, for future reference~~

EDIT: this patch is wrong, don't use it. Correct one here: #839 (comment)

diff --git a/dist/public/node/error-handler.js b/dist/public/node/error-handler.js
index 6cdb8f6605678926a856abb5725b0c702b5f9dfb..44a6a2ba99269b656fc7bc97aaf627a694089529 100644
--- a/dist/public/node/error-handler.js
+++ b/dist/public/node/error-handler.js
@@ -134,7 +134,7 @@ export async function registerCleanBugsnagErrorsFromWithinPlugins(config) {
     // Bugsnag have their own plug-ins that use this private field
     const bugsnagConfigProjectRoot = Bugsnag?._client?._config?.projectRoot ?? path.cwd();
     const projectRoot = path.normalizePath(bugsnagConfigProjectRoot);
-    const pluginLocations = await Promise.all(config.plugins.map(async (plugin) => {
+    const pluginLocations = await Promise.all([...config.plugins].map(async (plugin) => {
         const followSymlinks = await realpath(plugin.root);
         return { name: plugin.name, pluginPath: path.normalizePath(followSymlinks) };
     }));

Add this to your package.json

"resolutions": {
    "@shopify/[email protected]": "patch:@shopify/cli-kit@npm%3A3.50.0#./.yarn/patches/@shopify-cli-kit-npm-3.50.0-106200eaff.patch"
  }

danieltroger · 2023-10-20T08:50:45Z

Hmm, while the change I described above worked when doing it on the yarn unplug-ed package, loading the patch I described above I'm now getting this, which seems to be coming from @oclif/core?:

Error: Unsupported path type: undefined
    at NodePathFS.mapToBase (~/Documents/depict.ai/shopify/.pnp.cjs:24673:11)
    at NodePathFS.realpathPromise (~/Documents/depict.ai/shopify/.pnp.cjs:23507:68)
    at ~/Documents/depict.ai/shopify/.pnp.cjs:25106:29
    at file://~/Documents/depict.ai/shopify/.yarn/unplugged/@shopify-cli-kit-patch-5420f6dfeb/node_modules/@shopify/cli-kit/dist/public/node/error-handler.js:146:38
    at Array.map (<anonymous>)
    at registerCleanBugsnagErrorsFromWithinPlugins (file://~/Documents/depict.ai/shopify/.yarn/unplugged/@shopify-cli-kit-patch-5420f6dfeb/node_modules/@shopify/cli-kit/dist/public/node/error-handler.js:145:67)
    at async Dev.init (file://~/Documents/depict.ai/shopify/.yarn/unplugged/@shopify-cli-kit-patch-5420f6dfeb/node_modules/@shopify/cli-kit/dist/public/node/base-command.js:29:13)
    at async Dev._run (~/Documents/depict.ai/shopify/.yarn/cache/@oclif-core-npm-3.4.1-dev.0-f8d56c4f69-5277d77e0c.zip/node_modules/@oclif/core/lib/command.js:302:13)
    at async Config.runCommand (~/Documents/depict.ai/shopify/.yarn/cache/@oclif-core-npm-3.4.1-dev.0-f8d56c4f69-5277d77e0c.zip/node_modules/@oclif/core/lib/config/config.js:399:25)
    at async run (~/Documents/depict.ai/shopify/.yarn/cache/@oclif-core-npm-3.4.1-dev.0-f8d56c4f69-5277d77e0c.zip/node_modules/@oclif/core/lib/main.js:85:16)

danieltroger · 2023-10-20T09:01:55Z

@mdonnalley BTW, I think this is oclif code. It ate up the stack trace, which is highly annoying. I had to proxy console.error (which didn't really work because nodejs uses console.error inside of console.trace) and get a debugger to get the stack trace in the comment above, this is what it originally printed to the console Error: Unsupported path type: undefined

danieltroger · 2023-10-20T09:05:04Z

Sorry for the insane spam. It was all my fault. I forgot .values() in my patch above 🤦‍♂️

This would be the correct patch:

diff --git a/dist/public/node/error-handler.js b/dist/public/node/error-handler.js
index 6cdb8f6605678926a856abb5725b0c702b5f9dfb..44a6a2ba99269b656fc7bc97aaf627a694089529 100644
--- a/dist/public/node/error-handler.js
+++ b/dist/public/node/error-handler.js
@@ -134,7 +134,7 @@ export async function registerCleanBugsnagErrorsFromWithinPlugins(config) {
     // Bugsnag have their own plug-ins that use this private field
     const bugsnagConfigProjectRoot = Bugsnag?._client?._config?.projectRoot ?? path.cwd();
     const projectRoot = path.normalizePath(bugsnagConfigProjectRoot);
-    const pluginLocations = await Promise.all(config.plugins.map(async (plugin) => {
+    const pluginLocations = await Promise.all([...config.plugins.values()].map(async (plugin) => {
         const followSymlinks = await realpath(plugin.root);
         return { name: plugin.name, pluginPath: path.normalizePath(followSymlinks) };
     }));

danieltroger · 2023-10-20T09:08:40Z

Niiice, it works!

@mdonnalley in summary:

The @oclif/core part of fixing yarn berry for shopify seems to work, for me at least :)
Please still see feat: better support yarn PnP #839 (comment)
I'll try to open an issue on @shopify/cli-kit that they should soon update oclif and need to do the .values() change
Please look into the logging issue, it's not good if stack traces get eaten, it makes issues much harder to debug since it's not even clear what package an error is coming from

mdonnalley · 2023-10-23T14:29:49Z

@danieltroger Thanks for the feedback! I'll definitely take a look at the stack trace issue you mentioned.

Other than that, that would you consider this ready to merge?

danieltroger · 2023-10-23T16:58:07Z

@danieltroger Thanks for the feedback! I'll definitely take a look at the stack trace issue you mentioned.

Other than that, that would you consider this ready to merge?

Perfect. Yes, please merge! Thanks a lot for all the effort.

Would appreciate if you could let shopify know once there's a release ready: Shopify/cli#3004 (comment)

mdonnalley · 2023-10-23T19:15:05Z

@danieltroger we just published 3.6.0 with this fix. Thanks again for your help on this!

mdonnalley and others added 7 commits October 18, 2023 15:35

feat: support pnp plugins

8e217a6

refactor: improve implementation

914f148

feat: better support yarn PnP

c5d87fc

Merge branch 'main' into mdonnalley/pnp-plugins

eb15072

Merge branch 'main' into mdonnalley/pnp-plugins

a721d88

chore(release): 3.4.1-dev.0 [skip ci]

6baa39f

Merge branch 'main' into mdonnalley/pnp-plugins

d5dc324

fix: safely fail if pnpapi cannot be found

c2fc8d2

danieltroger reviewed Oct 20, 2023

View reviewed changes

danieltroger mentioned this pull request Oct 20, 2023

[Bug]: Yarn berry can't be used, please bump oclif version and change config.plugins to [...config.plugins.values()] Shopify/cli#3004

Closed

2 tasks

WillieRuemmele approved these changes Oct 23, 2023

View reviewed changes

WillieRuemmele merged commit 7efb8c5 into main Oct 23, 2023
34 checks passed

WillieRuemmele deleted the mdonnalley/pnp-plugins branch October 23, 2023 19:06

dotansimha mentioned this pull request Dec 13, 2023

Hive CLI with Yarn PnP graphql-hive/console#3596

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: better support yarn PnP #839

feat: better support yarn PnP #839

mdonnalley commented Oct 19, 2023

mdonnalley commented Oct 19, 2023 •

edited

Loading

danieltroger Oct 20, 2023 •

edited

Loading

mdonnalley Oct 23, 2023

danieltroger commented Oct 20, 2023

danieltroger commented Oct 20, 2023 •

edited

Loading

danieltroger commented Oct 20, 2023 •

edited

Loading

danieltroger commented Oct 20, 2023 •

edited

Loading

danieltroger commented Oct 20, 2023 •

edited

Loading

danieltroger commented Oct 20, 2023 •

edited

Loading

mdonnalley commented Oct 23, 2023

danieltroger commented Oct 23, 2023

mdonnalley commented Oct 23, 2023

feat: better support yarn PnP #839

feat: better support yarn PnP #839

Conversation

mdonnalley commented Oct 19, 2023

mdonnalley commented Oct 19, 2023 • edited Loading

danieltroger Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

mdonnalley Oct 23, 2023

Choose a reason for hiding this comment

danieltroger commented Oct 20, 2023

danieltroger commented Oct 20, 2023 • edited Loading

danieltroger commented Oct 20, 2023 • edited Loading

danieltroger commented Oct 20, 2023 • edited Loading

danieltroger commented Oct 20, 2023 • edited Loading

danieltroger commented Oct 20, 2023 • edited Loading

mdonnalley commented Oct 23, 2023

danieltroger commented Oct 23, 2023

mdonnalley commented Oct 23, 2023

mdonnalley commented Oct 19, 2023 •

edited

Loading

danieltroger Oct 20, 2023 •

edited

Loading

danieltroger commented Oct 20, 2023 •

edited

Loading

danieltroger commented Oct 20, 2023 •

edited

Loading

danieltroger commented Oct 20, 2023 •

edited

Loading

danieltroger commented Oct 20, 2023 •

edited

Loading

danieltroger commented Oct 20, 2023 •

edited

Loading