fix: don't start plugins for apps without a plugin entrypoint #850
Conversation
There was a problem hiding this comment.
@KaiVandivier: will you not have the similar problem if you have a plugin without an app entry point?
|
Good point... I'll fix that too |
|
Ran some tests to make sure it all works:
|
cli/src/commands/start.js
Outdated
| @@ -145,12 +163,12 @@ const handler = async ({ | |||
| startPromises.push(shell.start({ port: newPort })) | |||
| } | |||
|
|
|||
| if (shouldStartBoth || shouldStartOnlyPlugin) { | |||
| if (shouldStartPlugin) { | |||
| const pluginPort = shouldStartBoth ? newPort + 1 : newPort | |||
There was a problem hiding this comment.
should this be:
const pluginPort = shouldStartApp ? newPort + 1 : newPort ?
in case you indicated to start the app as well, but then did not have an entry point for it, then you could start the plugin on the newPort without incrementing? Or is the idea that you increment the port number to indicate that you should have been able to start the app (as you requested to do so 😅)
There was a problem hiding this comment.
Ah, a fine distinction -- I think you're right that shouldStartApp would be better 🙂
cli/src/commands/start.js
Outdated
| entryPoints.plugin && (shouldStartBoth || shouldStartOnlyPlugin) | ||
| if (!shouldStartApp && !shouldStartPlugin) { | ||
| throw new Error( | ||
| 'Nothing is configured to start. Check the start script and the configured entrypoints, then try again.' |
There was a problem hiding this comment.
I wonder if language like The requested app and/or plugin is not configured to start. would be slightly clearer than Nothing is configured to start? At the moment if you try running yarn start --plugin with only an app entry point, you'll get this message about "Nothing" being configured to start, which is true in the sense that Nothing you requested is configured, but there is still some configured entry point (you just haven't requested to start it).
PS I don't care much about this point as you only get here if you're not really paying attention to the parameters you're passing 😅
There was a problem hiding this comment.
That does sound clearer, I'll use that 🙂
|
Implemented and retested |
## [11.3.1](v11.3.0...v11.3.1) (2024-06-03) ### Bug Fixes * don't start plugins for apps without a plugin entrypoint ([#850](#850)) ([a89d4cf](a89d4cf))
|
🎉 This PR is included in version 11.3.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
# [12.0.0-alpha.2](v12.0.0-alpha.1...v12.0.0-alpha.2) (2024-06-20) ### Bug Fixes * clean up for plugins [LIBS-620] ([#851](#851)) ([13af3b5](13af3b5)) * do not encode username, password ([#852](#852)) ([2fb4272](2fb4272)) * don't start plugins for apps without a plugin entrypoint ([#850](#850)) ([a89d4cf](a89d4cf)) ### Features * parse pluginType from d2 config to add to manifest.webapp ([#849](#849)) ([c1dae23](c1dae23)) * start plugin and app separately [LIBS-391] [LIBS-392] ([#848](#848)) ([82003e7](82003e7))
|
🎉 This PR is included in version 12.0.0-alpha.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Missed this in the previous PR -- pointed out by Edoardo:
After: