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

Prepare for stylelint version 14 #233

Closed
6 tasks done
jeddy3 opened this issue Sep 26, 2021 · 10 comments · Fixed by #254
Closed
6 tasks done

Prepare for stylelint version 14 #233

jeddy3 opened this issue Sep 26, 2021 · 10 comments · Fixed by #254
Labels
pr: dependencies relates to dependencies
Milestone

Comments

@jeddy3
Copy link
Member

jeddy3 commented Sep 26, 2021

We're preparing a major release of stylelint. There are quite a few changes, but only some of which will impact this extension. I want to share my thoughts on what changes may be needed so that it's in the back of your mind as you continue to improve things like you did in #224 and #228.

The biggest changes we've made is removing the syntax option and no longer bundling syntaxes in stylelint itself. Users will need to extend stylelint (via shared-configs, plugins and custom syntaxes) to lint something other than vanilla CSS.

I think this will be a good thing for users of this extension as they are often confused by stylelint linting (or attempting to lint and failing) their JavaScript and TypeScript files.

I think this, along with some of other breaking changes, will mean we need to:

  • Change the default of the validate option to ["css", "postcss"]
  • Document that extension will only automatically validate documents with css and postcss language identifiers
  • Document that users will need to explicitly opt-in to other languages using the validate option
  • Change the default of the snippet option to ["css", "postcss"]
  • Remove the configOverrides option
  • Remove the syntax option

Do those suggestions seem about right to you? I'm not familiar with this extension's codebase (nor best practice when it comes to breaking changes in extensions), so please don't hesitate to suggest alternatives!

There's no firm release date for version 14 yet, but we're getting closer. All but one of the changes to stylelint itself are done, but we'd like to see some more custom syntaxes (e.g. postcss-html) and shared-configs (e.g. stylelint-config-recommended-scss) made compatible before we release. So, it may be best to continue to make your improvements and releasing those, before cutting a major release for these breaking changes. What do you think?

Many thanks again for contributing to this extension! It's fantastic to see it get some attention.

@ota-meshi
Copy link
Member

I think those changes will probably need to drop support for stylelint<=v13.
I think this extension also needs to release v1 with breaking changes.

@jeddy3
Copy link
Member Author

jeddy3 commented Sep 26, 2021

I think those changes will probably need to drop support for stylelint<=v13.

In stylelint 14 itself were supporting:

{ 
  "engines": {
    "node": "^12.20.0 || ^14.13.1 || >=16.0.0"
  }
}

I don't know how node versions work for extensions, though.

I think this extension also needs to release v1 with breaking changes

👍

@ota-meshi
Copy link
Member

The support version of node does not affect whether to drop v13 support.
Perhaps removing some options for this extension is confusing for v13 users because it can't do the same as the CLI.
However, leaving those options will not work, which will confuse v14 users.
Limiting stylelint support to >=v14 will probably make it easier to use, document, and maintain. If not, I think they will be complicated.

@jeddy3
Copy link
Member Author

jeddy3 commented Sep 26, 2021

Whoops, crossed wires. I misread and thought you were talking about node versions.

Limiting stylelint support to >=v14 will probably make it easier to use, document, and maintain. If not, I think they will be complicated.

Yes, I agree. Only supporting stylelint version 14 and up (and dropping version 13 support) as a breaking change is the way to go.

@adalinesimonian
Copy link
Member

adalinesimonian commented Sep 27, 2021

I entirely agree that 1.x should track 14 and drop support for 13 and prior. I want to push out one last 0.x release with the latest 13.x stylelint (#234) and document formatting support (#25) for those users still on 13. I don't think it'll add much work to our plate; it's the only task remaining before 0.87.0 can be released. I've already started working on it a few days ago! In the meantime we can upgrade this extension for 14.x, exciting!

@adalinesimonian
Copy link
Member

I want to push out one last 0.x release with the latest 13.x stylelint (#234) and document formatting support (#25) for those users still on 13.

I just published v0.87.1, so that is out of the way now!

@adalinesimonian
Copy link
Member

Just created a v1 branch and configured CI to run for it! This way we can easily maintain 0.x until 1.x is ready for prime-time.

@adalinesimonian adalinesimonian added status: wip is being worked on by someone upstream relates to an upstream package labels Oct 4, 2021
@jeddy3
Copy link
Member Author

jeddy3 commented Oct 6, 2021

Just created a v1 branch and configured CI to run for it! This way we can easily maintain 0.x until 1.x is ready for prime-time.

Fantastic work!

It's also amazing that you managed to bundle the 0.x branch, especially as it includes stylelint 13.x and all the problematic syntax stuff. I was offline this week, and I'm sorry I wasn't able to warn you about the hurdles down that path. Fortunately, things will be better in the future:

@adalinesimonian
Copy link
Member

It's also amazing that you managed to bundle the 0.x branch, especially as it includes stylelint 13.x and all the problematic syntax stuff. I was offline this week, and I'm sorry I wasn't able to warn you about the hurdles down that path.

It surprised me when I saw all the users in issues reporting bug after bug for a few days, it kept me quite busy. I had a lot of faith in the test suites, but as I've been working more and more on the project, I've become much more familiar with the large gap in coverage. I am working in the v1 branch on getting the codebase to be much easier to test and getting tests to be way more thorough, as well as bringing in Stylelint 14.

Fortunately, things will be better in the future:

...

  • the syntax stuff has been removed in favour of explicit configurations

Thank god! I've never been happier to see a feature fall off the earth.

Let me know if there's anything I can share from my experience bundling it downstream in this extension.

@jeddy3
Copy link
Member Author

jeddy3 commented Oct 7, 2021

I am working in the v1 branch on getting the codebase to be much easier to test and getting tests to be way more thorough

That's great to hear!

Let me know if there's anything I can share from my experience bundling it downstream in this extension.

Thanks! It looks like ESBuild can bundle the v14 branch without issue, so I hope it'll be plain sailing. Will ask if we run into issues, though!

adalinesimonian added a commit that referenced this issue Oct 8, 2021
Closes #233

- Stylelint dependency changed to point to v14 branch on GitHub (Will
  need to be changed to the packaged version once Stylelint 14 is
  published to npm.)
- Default values for `validate` and `snippet` options set to
  `['css', 'postcss']`
- Removed `syntax` option
- Removed `configOverrides` option
- Documented changes and provided migration path in readme
- Made appropriate changes in tests
- Removed tests for removed features
@adalinesimonian adalinesimonian linked a pull request Oct 8, 2021 that will close this issue
adalinesimonian added a commit that referenced this issue Oct 10, 2021
* feat!: Upgrade to Stylelint 14 🎉

Closes #233

- Stylelint dependency changed to point to v14 branch on GitHub (Will
  need to be changed to the packaged version once Stylelint 14 is
  published to npm.)
- Default values for `validate` and `snippet` options set to
  `['css', 'postcss']`
- Removed `syntax` option
- Removed `configOverrides` option
- Documented changes and provided migration path in readme
- Made appropriate changes in tests
- Removed tests for removed features

* fix: fix links to sections in readme

* docs: various improvements to readme

* fix: fix section link in readme

* docs: updates suggested by @jeddy3

* test: remove html and markdown tests

TODO: Restore once postcss-html and postcss-markdown are PostCSS 8
compatible.

re: #254 (comment)
@adalinesimonian adalinesimonian added pr: dependencies relates to dependencies Planning and removed status: wip is being worked on by someone upstream relates to an upstream package labels Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: dependencies relates to dependencies
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants