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

add parsing support for supports conditions #530

Conversation

romainmenke
Copy link
Collaborator

@romainmenke romainmenke commented Jul 14, 2023

see : #529

  • added parsing support for supports conditions
  • rewrite the import statement parser

Fully implementing supports conditions will require a fairly large refactor and the codebase is already very complex.

I would like to first do a few refactors.
This is one of them.

I've added c8 to ensure that test coverage remains high throughout this process.

File % Stmts % Branch % Funcs % Lines Uncovered Line #s
All files 100 99.65 100 100
postcss-import 100 100 100 100
index.js 100 100 100 100
postcss-import/lib 100 99.34 100 100
assign-layer-names.js 100 100 100 100
data-url.js 100 100 100 100
join-layer.js 100 100 100 100
join-media.js 100 100 100 100
load-content.js 100 100 100 100
parse-statements.js 100 100 100 100
process-content.js 100 93.75 100 100 26
resolve-id.js 100 100 100 100

Initially the parsing algorithm was a simple split:

  • look for anything that could be a uri
  • everything after is a media condition

With layer and supports something more flexible is needed.
I've changed the parsing algorithm to a loop that keeps track of what has been encountered and errors on any unexpected sequences.


Any supports conditions will give errors:

Supports conditions are not implemented at this time.

This is a behavior change.
Before this PR supports conditions would be treated as if they were media conditions, which is also incorrect.

Both behaviors are "broken" but after this change CSS authors will receive more meaningful feedback.

@romainmenke romainmenke changed the title add parsing support for supports conditions add parsing support for supports conditions Jul 14, 2023
@RyanZim
Copy link
Collaborator

RyanZim commented Jul 15, 2023

@romainmenke are you thinking we should release with this warning for @supports, or is this just a temporary measure until we add support after refactoring?

lib/parse-statements.js Show resolved Hide resolved
lib/parse-statements.js Show resolved Hide resolved
@romainmenke
Copy link
Collaborator Author

romainmenke commented Jul 15, 2023

are you thinking we should release with this warning for @supports, or is this just a temporary measure until we add support after refactoring?

I am hoping it is temporary.
But I want to keep the code base healthy after each change and I would like to keep each change small-ish.

The state in this pull request is something that can be released if needed.

I plan to continue work towards full support for supports conditions, but this could take a while. It really complicates things that four different types of rules interact (import, media, layer, supports). With layer I was able to patch a few existing bits and make it work, but a larger refactor is needed for supports.

@romainmenke romainmenke mentioned this pull request Jul 19, 2023
3 tasks
Copy link
Collaborator

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

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

Let's split the addition of c8 into its own PR.

Code here LGTM.

CHANGELOG.md Outdated Show resolved Hide resolved
@romainmenke
Copy link
Collaborator Author

Thank you for the review 🙇

@romainmenke romainmenke merged commit 0aaed50 into master Jul 20, 2023
3 checks passed
@romainmenke romainmenke deleted the add-parsing-support-for-at-supports--empathetic-guppy-fe6c60e290 branch July 20, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants