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

fix: move webpack to a peer dependency and allow webpack 2 #46

Merged
merged 6 commits into from
Jan 5, 2017

Conversation

mattlewis92
Copy link
Member

@mattlewis92 mattlewis92 commented Dec 17, 2016

Using this plugin with webpack 2 currently installs 2 versions of webpack.

Resolves #38.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.238% when pulling a4d9ff8 on mattlewis92:patch-1 into e6cf081 on vieron:master.

Copy link
Contributor

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. The only thing I'm wondering about now is how can we test against both webpack v1 and v2 so that it ships and is working for both?

@@ -23,14 +23,14 @@
},
"homepage": "https://github.com/vieron/stylelint-webpack-plugin#readme",
"peerDependencies": {
"stylelint": "^7.0.1"
"stylelint": "^7.0.1",
"webpack": "^2 || ^2.2.0-rc.0 || ^2.1.0-beta.0 || ^1.13.2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather just have ^2.0.0 || ^1.13.2 here... I would think it's better to just "officially" support stable versions. With npm >=3 or yarn you won't get peerDeps installed anyway so you can still choose what version you want.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no breaking changes between webpack 1 and webpack 2's API that would affect this plugin, it's very safe to support the beta versions of webpack, and gets rid of the annoying peer dependency warning on install.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, because the rc and beta versions do not fall under semver technically so they aren't resolved by ^2.0.0? Fair enough.

@coveralls
Copy link

coveralls commented Dec 18, 2016

Coverage Status

Coverage remained the same at 95.238% when pulling 8789da9 on mattlewis92:patch-1 into e6cf081 on vieron:master.

@mattlewis92
Copy link
Member Author

@JaKXz I've added a simple npm script to test against both v1 and v2 of webpack, LMK what you think.

@mattlewis92
Copy link
Member Author

mattlewis92 commented Dec 18, 2016 via email

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.238% when pulling a13d6ca on mattlewis92:patch-1 into 6b74bb6 on vieron:master.

@mattlewis92
Copy link
Member Author

@JaKXz I just spotted there was a merge conflict with master which I've now resolved, would you like me to make any more changes before this can be merged? 😃

@JaKXz
Copy link
Contributor

JaKXz commented Dec 27, 2016

@mattlewis92 thanks for fixing that!

I'm still not sure about the way to test the different versions of webpack though. Maybe this should be a bigger discussion with @sokra and other plugin maintainers who want to maintain backwards compatibility?

@mattlewis92
Copy link
Member Author

Given that webpack 2 is now in RC and there are no more breaking changes planned (and previously none that have affected this plugin) - I wouldn't worry too much about it. AFAIK the plugin / loader API will always be pretty stable to avoid breaking the webpack ecosystem - the majority of breaking changes are in the EU's webpack config itself. Maybe @TheLarkInn can give some more insight into this though?

@JaKXz
Copy link
Contributor

JaKXz commented Jan 5, 2017

@mattlewis92 thank you for your patience on this!

I had a look at the long thread that is npm/npm#5499 and found: https://github.com/scott113341/npm-install-version

I think a programmatic solution with npm-install-version is the way to go - at first glance it looks like a sustainable way to test against both versions of webpack until webpack 1 support is dropped, which of course isn't happening any time soon.

I really appreciate your work on this - let me know if you want to give that a shot or I can take it on if you're spent. :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 95.455% when pulling a9d6b2b on mattlewis92:patch-1 into 6b74bb6 on vieron:master.

@mattlewis92
Copy link
Member Author

@JaKXz I've done a basic implementation using npm-install-version - let me know if that's OK for you

@coveralls
Copy link

coveralls commented Jan 5, 2017

Coverage Status

Coverage increased (+0.2%) to 95.455% when pulling cadc39a on mattlewis92:patch-1 into 6b74bb6 on vieron:master.

@JaKXz JaKXz merged commit fa7ba5a into webpack-contrib:master Jan 5, 2017
@JaKXz
Copy link
Contributor

JaKXz commented Jan 8, 2017

+ stylelint-webpack-plugin@0.5.0 published! thanks again!

@mattlewis92 mattlewis92 deleted the patch-1 branch January 9, 2017 08:56
@mattlewis92
Copy link
Member Author

Awesome, thanks! 😃

joshwiens pushed a commit that referenced this pull request Mar 31, 2018
* test: test against both webpack 1 and webpack 2

* chore: use npm install version

* style: whitespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants