Skip to content
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

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

GretaD
Copy link
Contributor

@GretaD GretaD commented Feb 14, 2023

fixes #3696

@GretaD GretaD added the 2. developing Work in progress label Feb 14, 2023
@GretaD GretaD self-assigned this Feb 14, 2023
@GretaD
Copy link
Contributor Author

GretaD commented Feb 14, 2023

i cannot run npm right now, will check tomorrow again

@GretaD GretaD force-pushed the dependencies/node-module-webpack branch from c39d097 to 0a4d092 Compare February 15, 2023 10:21
@GretaD GretaD added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 15, 2023
@GretaD GretaD marked this pull request as ready for review February 15, 2023 10:30
@ChristophWurst
Copy link
Contributor

I think this should be a peer dependency

Otherwise 👍

@GretaD
Copy link
Contributor Author

GretaD commented Feb 16, 2023

I think this should be a peer dependency

Otherwise +1

right, but we only have dependencies and devDependencies here, so should i switch to Dependencies?

@ChristophWurst
Copy link
Contributor

peerDependencies

@GretaD GretaD force-pushed the dependencies/node-module-webpack branch from cc72485 to 247e904 Compare February 16, 2023 18:31
@raimund-schluessler
Copy link
Contributor

peerDependencies

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.

@ChristophWurst
Copy link
Contributor

npm installs peers automatically, doesn't it?

adding it as dev dep is also fine 👍

@raimund-schluessler
Copy link
Contributor

npm installs peers automatically, doesn't it?

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.

@GretaD GretaD force-pushed the dependencies/node-module-webpack branch from 3d93cad to 75bd88a Compare February 17, 2023 12:58
Copy link
Contributor

@raimund-schluessler raimund-schluessler left a 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 🙂🙈

@GretaD
Copy link
Contributor Author

GretaD commented Feb 17, 2023

@GretaD You put it in devDependencies now. I think this will not work. And please order it alphabetically slightly_smiling_facesee_no_evil

yes, thats what christoph proposed and you did not object on your comment. Changing it again then

@raimund-schluessler
Copy link
Contributor

@GretaD You put it in devDependencies now. I think this will not work. And please order it alphabetically slightly_smiling_facesee_no_evil

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?

@GretaD
Copy link
Contributor Author

GretaD commented Feb 17, 2023

@GretaD You put it in devDependencies now. I think this will not work. And please order it alphabetically slightly_smiling_facesee_no_evil

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>
@raimund-schluessler
Copy link
Contributor

Then let's hope adding it as a dependency works 🙂 🙈

@raimund-schluessler raimund-schluessler merged commit a388f53 into master Feb 17, 2023
@raimund-schluessler raimund-schluessler deleted the dependencies/node-module-webpack branch February 17, 2023 17:52
@szaimen szaimen mentioned this pull request Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing dependencies on node modules
3 participants