-
Notifications
You must be signed in to change notification settings - Fork 218
Fix multiple tunnel plugins error #6554
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
Conversation
Coverage report
Show files with reduced coverage 🔻
Test suite run success3322 tests passing in 1360 suites. Report generated by 🧪jest coverage report action from 29bd2f3 |
This comment was marked as outdated.
This comment was marked as outdated.
amcaplan
left a comment
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.
Checking my understanding: This new logic means that if I have pinned @shopify/app in my package.json, while it might previously have preferred that as the source of command implementations, now it will prefer the version that comes bundled in @shopify/cli?
If so, that seems like a good idea, but I wanted to be sure I understood correctly.
|
@amcaplan this is not related to the I'm not sure about how some people are getting into this situation. The only way I found to reproduce it is to install the plugin with |
|
@isaacroldan where exactly would you add it? I think we don't have the config in prerun, which is what we need to update. |
|
@gonzaloriestra you do have config in prerun |
|
Already discussed it with Isaac and this should be safe because:
But we are hiding a weird situation: the users shouldn't have those plugins. I think I prefer to keep raising an error, but handled and explaining how to solve it: #6555 @amcaplan what do you prefer? |
|
I think we can start by rescuing the situation (because it's not known to be unsafe) but starting to warn users, and eventually we drop support for this entirely and leave it as a handled error. |
|
/snapit |
|
🫰✨ Thanks @gonzaloriestra! Your snapshot has been published to npm. Test the snapshot by installing your package globally: npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20251030155921Caution After installing, validate the version by running just |
dmerand
left a comment
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.
I'm aligned with this approach.
and eventually we drop support for this entirely and leave it as a handled error.
How do we ensure this happens?
We don't have a set process for this. I added a reminder to our Slack channel for two months from now. |
WHY are these changes introduced?
Fixes https://github.com/shop/issues-develop/issues/21321
WHAT is this pull request doing?
Ignores the plugins
@shopify/appor@shopify/plugin-cloudflarebecause they are already included in the bundle.That way, the
multiple tunnel pluginserror won't appear when for some reason the cloudflare plugin is duplicated.Also, it shows a warning so the users can remove those plugins that may cause conflicts:
How to test your changes?
npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20251030155921shopify plugins add @shopify/appshopify app devMeasuring impact
How do we know this change was effective? Please choose one:
Checklist