-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Addon-docs: Make babel-loader an optional peer dependency #19385
Conversation
d81355a
to
4e1782d
Compare
Thanks @IanVS ! Seems like @ndelangen has an interesting idea, can you please discuss with him and possibly @shilman ? |
I think we should not have more peerDependencies, they do not actually fix the issue. What we should do instead is have the reference to the babel loader come from a preset, probably the builder. If the builder depends on // builder/preset.js
export const babelLoaderRef = () => require.resolve('babel-loader'); in another place (docs addons): // addons/docs/preset.js
const webpack = (config, options) => {
config.module.rules.push({
use: await options.presets.apply('babelLoaderRef'),
});
return config;
}
|
I think that could work for this case, but it would also be great if we could find a more general solution. I'm hitting the same kind of issue in storybookjs/addon-svelte-csf#72, where |
The CLI has a concept of |
@IanVS I expanded upon my comment, maybe you hadn't seen it / got no notification, so just doing a ping here, in case. |
@ndelangen do you envision we would follow this pattern for any and all dependencies that are specific to one of the builders for all addons, or just for babel-loader? |
I suppose the other option is to publish a webpack version and a vite version of such addons. They could have some common functionality shared between then, but publish separate packages with different dependencies and exports. It would be a shame, but might be the cleanest way to avoid the extra dependencies. |
At least for Svelte, a project won't work without |
# Conflicts: # code/addons/docs/package.json
We discussed this, and decided to take the approach of I think there's a good chance that using an optional peerDependency would also work here, but we've been bitten by peer deps in the past, so I don't blame folks for wanting to avoid them. |
Socket Security Report👍 No new dependency issues detected in pull request Socket.dev scan summary
Bot CommandsTo ignore an alert, reply with a comment starting with Powered by socket.dev |
I also removed |
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.
LGTM! thanks for your patience @IanVS and help @ndelangen ❤️
Issue:
What I did
The webpack
babel-loader
was listed as a dependency ofaddon-docs
, and a required peer dependency ofrenderers/vue
andrenderers/vue3
. Since all of these can be used with vite, which does not requirebabel-loader
, I've moved them to optional peer dependencies.I suggest that if necessary,
babel-loader
could be added as a dependency or peer dependency of the webpack frameworks if we want to ensure that it's available for addons.How to test