-
Notifications
You must be signed in to change notification settings - Fork 85
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
Add node-polyfill-webpack-plugin #3752
Conversation
|
c39d097
to
0a4d092
Compare
I think this should be a peer dependency Otherwise 👍 |
right, but we only have dependencies and devDependencies here, so should i switch to Dependencies? |
peerDependencies |
cc72485
to
247e904
Compare
While technically correct, listing this as a peer dependency is very likely a breaking change. Every app using nextcloud/vue will have to have this peer dependency in their dependency list. Otherwise npm will probably warn about it. I don't know how that behaves when node-polyfill-webpack-plugin is already installed as a dependency of another package (e.g. nextcloud/webpack-config), though. But to keep it pragmatic, I would just list it as a dependency, to be honest. |
npm installs peers automatically, doesn't it? adding it as dev dep is also fine 👍 |
Indeed, it seems this feature came back with npm v7, it was gone since npm v3. As npm v7 is the lowest supported version here, it's probably fine. But I didn't test it. |
3d93cad
to
75bd88a
Compare
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.
@GretaD You put it in devDependencies
now. I think this will not work. And please order it alphabetically 🙂🙈
yes, thats what christoph proposed and you did not object on your comment. Changing it again then |
Ah, yes, sorry. I missed this. I think a development dependency will not work as they are afaik not installed for the peer. Did you try it with a dev dep and it worked? |
no i didnt test it on devDep, sorry 🙈 |
Signed-off-by: greta <gretadoci@gmail.com>
fbcb2d2
to
5464b8d
Compare
Then let's hope adding it as a dependency works 🙂 🙈 |
fixes #3696