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

refactor: Update to PostCSS 8. #151

Merged
merged 2 commits into from
Oct 21, 2020
Merged

Conversation

ludofischer
Copy link
Contributor

Fixes: #150

  • chore(deps): bump postcss to latest version.
  • chore(deps): bump postcss-parser-tests to latest version.
  • refactor(tests): use new postcss-parser-tests exports.

BREAKING CHANGE: drop Node 8 support, as PostCSS 8 requires Node >= 10.

Which issue # if any, does this resolve?
#150

Please check one:

  • New tests created for this change
  • Tests updated for this change

This PR:

  • Adds new API
  • Extends existing API, backwards-compatible
  • Introduces a breaking change
  • Fixes a bug

I’ve looked at this this commit to get an idea of the required changes. In the end only the tests needed to change imports. In theory you could release a version that’s compatible with both PostCSS 7 and 8, if you kept a separate test suite.

Fixes: shellscape#150

* chore(deps): bump `postcss` to latest version.
* chore(deps): bump `postcss-parser-tests` to latest version.
* refactor(tests): use new `postcss-parser-tests` exports.

BREAKING CHANGE: drop Node 8 support, as PostCSS 8 requires Node >= 10.
@ludofischer
Copy link
Contributor Author

It looks like the CircleCI tests fails at the security checks because npm audit reports some vulnerabilities in the dev dependencies.

@shellscape
Copy link
Owner

Thanks for putting this together. I'll get that audit fixed up for you here shortly.

@shellscape
Copy link
Owner

Updated. Please merge from upstream/master.

@ludofischer
Copy link
Contributor Author

Merged upstream, Node 8 tests fail as expected (latest ava drops Node 8 support).

@shellscape
Copy link
Owner

Gah. That's right. I'll clean up the CI config for you here.

package.json Show resolved Hide resolved
@ludofischer
Copy link
Contributor Author

@shellscape One PostCSS maintainer (@ai) suggested on Gitter to copy the postcss/lib/tokenize.js into the parser’s own tree instead of importing it, as it’s not meant to be a public API and relying on it has caused breakage in the past. What do you think about this?

@shellscape
Copy link
Owner

shellscape commented Oct 21, 2020

@ludofischer I have a great deal of respect for Andrey, and he's helped me and other postcss-related projects out more times than I can count. But on this particular issue, he and I disagree. I adopted use of the internal classes to ease the burden of maintaining a separate parser and tokenizer. Even if the base classes in PostCSS change, it's still a trivial update in this package. Whereas maintaining both myself was a massive burden rife with edge cases that had already been accounted for in the PostCSS base classes. Copy/pasting theirs into this project also seemed futile to me and I ruled that out.

All in all, I'm happy to update on my end when the base classes change. And as this is mean to be a development-time-only package, I don't see an issue with two separate copies of PostCSS, should a parent package/project use a newer version that's out of range with the one that postcss-less depends upon.

@shellscape shellscape merged commit a168001 into shellscape:master Oct 21, 2020
@shellscape
Copy link
Owner

thanks!

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.

PostCSS 8
3 participants