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

Restructuring with linting and coverage #17

Merged
merged 11 commits into from
Aug 2, 2016
Merged

Restructuring with linting and coverage #17

merged 11 commits into from
Aug 2, 2016

Conversation

JaKXz
Copy link
Contributor

@JaKXz JaKXz commented Jul 28, 2016

Fixes #14.
Closes #15.

Hi @vieron, while writing #15 I was having some trouble debugging so I went a bit crazy and restructured things. I figured I should at least separate these things into different PRs so we can get the update/fix merged and published.

This might make for a good 1.0.0 so that you know where the coverage stands and so on.

EDIT: I'd definitely recommend only squash-merging EITHER #15 or this. Thanks!

@vieron
Copy link
Contributor

vieron commented Aug 2, 2016

Hi @JaKXz!

Thank you very much for all the work! And sorry for the late response, I took some days off.

I'm in favour of merging this PR, but before that there is one thing I would like to change: Why removing stylelint from peerDependencies?

My suggestion is to add it back with a semver range, like "stylelint": "^6.0.0 || ^7.0.0". As far as I've seen the breaking changes are all related with stylelint rules and not with the JavaScript API

What do you think?

Thanks again!

@JaKXz
Copy link
Contributor Author

JaKXz commented Aug 2, 2016

@vieron no worries, glad you're back :)

I think removing peerDependencies is a good idea for two reasons:

  1. It's "deprecated" in npm v3: meaning the specified modules aren't installed implicitly anyway. It seems like a reasonable requirement that if you have stylelint-webpack-plugin installed you must have stylelint installed as well.
  2. Depending on stylelint v6 OR v7 is too much maintenance, because there are breaking changes between these versions... so we'd have to keep multiple versions of the .stylelintrc in the tests to make sure the API isn't throwing the wrong errors.

EDIT: The good thing is that this can be release v0.3.0 which will be a breaking change for those with "stylelint-webpack-plugin": "^0.2.0" installed.

@vieron
Copy link
Contributor

vieron commented Aug 2, 2016

@JaKXz as you said peerDependencies is not deprecated, it just works different than before. Packages are not installed but you get a warning, which is good.

And regarding the second point, for me, it also applies when not specifying a stylelint version. You rely on any version the user has installed. It's like saying all versions are supported, but we just run test against v7.

I still think we should add the peerDependency, just depending in ^7.0.0 or in both ^6.0.0 || ^7.0.0.

@JaKXz
Copy link
Contributor Author

JaKXz commented Aug 2, 2016

Fair enough, good point - I'll add it back and merge :)

@JaKXz JaKXz merged commit 0d71338 into webpack-contrib:master Aug 2, 2016
@JaKXz JaKXz deleted the chore/restructure-lint-coverage branch August 2, 2016 21:59
@vieron
Copy link
Contributor

vieron commented Aug 4, 2016

Published 0.3.0 to npm :)

joshwiens pushed a commit that referenced this pull request Mar 31, 2018
* Update stylelint dependency

* Fix renamed rules in stylelint v7

* Add editorconfig

* Simplify linter

* Use chai array length assertion

* Restructure with linting and coverage

* Add some documentation for runCompilation and add missing use strict

* Keep stylelint v7 in peerDependencies
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.

Update stylelint dependency
2 participants