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

Stylelint v14 Compatibility #197

Merged
merged 3 commits into from
Oct 29, 2021
Merged

Conversation

gfyoung
Copy link
Contributor

@gfyoung gfyoung commented Oct 21, 2021

Updates will also supersede the following PRs:

Closes: #195
Closes: #196

* stylelint-order: 4.0.0 --> 5.0.0
* stylelint-scss: 3.18.0 --> 4.0.0
* stylelint: 13.7.0 --> 14.0.0
* tape: 5.0.1 --> 5.3.1

For compatibility with stylelint@v14:

* Sync "node" compatibility

* (tape) Add "postcss-scss" as devDependency
* (tape) Update test in "url-quotes.js"

Closes: bjankord#195
Closes: bjankord#196
@stof
Copy link
Contributor

stof commented Oct 26, 2021

Not having changes to use the new customSyntax option of stylelint looks suspicious to me.

@gfyoung
Copy link
Contributor Author

gfyoung commented Oct 26, 2021

Given that this repository is pretty much a configuration of existing rules, I don't see where it would be needed.

@stof
Copy link
Contributor

stof commented Oct 26, 2021

Well, in stylelint 14, stylelint does not support parsing scss files if the config does not configure the customSyntax (see the migration guide).
and the config in this repository does not extend the one from stylelint-config-recommended-scss that would do it already, so it needs to provide that config, like this: https://github.com/stylelint-scss/stylelint-config-recommended-scss/blob/d373c8728cd130a7635173f19ce3bedf55eb9178/index.js#L5

@gfyoung
Copy link
Contributor Author

gfyoung commented Oct 26, 2021

True, but I would expect that the end user configures customSyntax in their .stylelintrc file.

I generally would not rely on the library to do that for me.

@stof
Copy link
Contributor

stof commented Oct 26, 2021

Well, this config cannot be used without it. So why would it force the user to do it themselves ? That's the opposite of reusable configs.

@stof
Copy link
Contributor

stof commented Oct 26, 2021

And having the config specific for a syntax specify that customSyntax is the official stylelint recommendation for config authors (which is why stylelint-config-recommended-scss does it)

@gfyoung
Copy link
Contributor Author

gfyoung commented Oct 26, 2021

Fair enough. Pushed a commit to address.

package.json Show resolved Hide resolved
israelKusayev pushed a commit to reflexology/client-V2 that referenced this pull request Oct 27, 2021
israelKusayev pushed a commit to reflexology/client-V2 that referenced this pull request Oct 27, 2021
@bjankord
Copy link
Owner

This looks good to me, all tests pass. Thanks for the PR @gfyoung and thanks for the discussion @stof

The node 10 tests failing is expected since we're dropping that from the engines list. I'll work on updating the GitHub Actions to no longer run tests on node 10. I'll plan on getting a new major version for this repo cut soon.

@bjankord bjankord merged commit 0fe0f3e into bjankord:main Oct 29, 2021
@gfyoung gfyoung deleted the stylelint-v14 branch October 29, 2021 10:16
@bjankord
Copy link
Owner

This has been released in v9.0.0. I also added the hacktoberfest topic to this repo, so if you are participating in that, you should get credit for this PR. Thanks again for the contribution @gfyoung and for the code review @stof!

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