-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
fix(package): add webpack >= v4.0.0
(peerDependencies
)
#61
Conversation
Webpack 4 alpha has been released. In preparation for v4, it seems reasonable to change this to allow *. This should cut down on maintenance and avoid unnecessarily blocking folks from updating going forward.
Oh actually it seems that * doesn't allow pre-releases :( |
webpack >= v4.0.0
as a peerDependency
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.
From a quick look at the code there shouldn't be anything affected by breaking changes between webpack v3.0.0...v4.0.0
in this loader. Did you test it locally within a project ?
@@ -21,7 +21,7 @@ | |||
}, | |||
"homepage": "https://github.com/webpack-contrib/expose-loader", | |||
"peerDependencies": { | |||
"webpack": "^2.0.0 || ^3.0.0" | |||
"webpack": "*" |
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.
- "webpack": "^2.0.0 || ^3.0.0"
+ "webpack": "^2.0.0 || ^3.0.0 || ^4.0.0-alpha.0"
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.
What do you think about removing this peerDependency altogether? I noticed that a bunch of our webpack-related dependencies have gone that route.
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 can't decide that, but I agree that the the peerDependency
is often just noise atm and maybe should just be set on loaders/plugins that really need it. On the other hand we are trying to bring in consistency across webpack-contrib
about things like that, but simply aren't there yet :)
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.
cc @d3viant0ne
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 have not tested it out locally yet, but maybe @gdborton has?
@@ -21,7 +21,7 @@ | |||
}, | |||
"homepage": "https://github.com/webpack-contrib/expose-loader", | |||
"peerDependencies": { | |||
"webpack": "^2.0.0 || ^3.0.0" | |||
"webpack": "*" |
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.
What do you think about removing this peerDependency altogether? I noticed that a bunch of our webpack-related dependencies have gone that route.
webpack >= v4.0.0
as a peerDependency
webpack >= v4.0.0
(peerDependencies
)
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.
Instead of a wildcard *
could you update the peerDependency
on webpack
to "webpack": "^2.0.0 || ^3.0.0 || ^4.0.0"
please, so I can patch this package asap ? :)
@michael-ciniawsky maybe better recreate PR? |
Maybe yes, but wanted to give a change :P |
@michael-ciniawsky let's wait to tomorrow 👍 |
Webpack 4 alpha has been released. In preparation for v4,
it seems reasonable to change this to allow *. This should cut
down on maintenance and avoid unnecessarily blocking folks
from updating going forward.