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 to replace postcss-media-query-parser with @csstools/media-query-list-parser #6848

Closed
9 tasks done
ybiquitous opened this issue May 16, 2023 · 5 comments · Fixed by #7021
Closed
9 tasks done
Labels
status: wip is being worked on by someone

Comments

@ybiquitous
Copy link
Member

ybiquitous commented May 16, 2023

Applicable rules:


While postcss-media-query-parser is old and unmaintained, @csstools/media-query-list-parser supports modern syntaxes and is used now in this project. Therefore, I believe we can completely switch.

Name Repo npm Last published
postcss-media-query-parser https://github.com/dryoma/postcss-media-query-parser https://www.npmjs.com/package/postcss-media-query-parser Oct 27, 2016
@csstools/media-query-list-parser https://github.com/csstools/postcss-plugins/tree/HEAD/packages/media-query-list-parser https://www.npmjs.com/package/@csstools/media-query-list-parser April 10, 2023

The current usage:

$ git grep -w 'postcss-media-query-parser' lib/
lib/rules/media-feature-name-allowed-list/index.js:8:const mediaParser = require('postcss-media-query-parser').default;
lib/rules/media-feature-name-case/index.js:7:const mediaParser = require('postcss-media-query-parser').default;
lib/rules/media-feature-name-disallowed-list/index.js:8:const mediaParser = require('postcss-media-query-parser').default;
lib/rules/media-feature-name-no-unknown/index.js:8:const mediaParser = require('postcss-media-query-parser').default;
lib/rules/media-feature-name-value-allowed-list/index.js:3:const mediaParser = require('postcss-media-query-parser').default;
lib/rules/no-duplicate-at-import-rules/index.js:3:const mediaParser = require('postcss-media-query-parser').default;
lib/rules/rangeContextNodeParser.js:22: * @param {import('postcss-media-query-parser').Node} node
lib/rules/unit-disallowed-list/index.js:6:const mediaParser = require('postcss-media-query-parser').default;
lib/rules/unit-disallowed-list/index.js:29: * @param {import('postcss-media-query-parser').Child} mediaFeatureNode
lib/rules/unit-no-unknown/index.js:9:const mediaParser = require('postcss-media-query-parser').default;

@romainmenke Could you please comment if you have any concerns?

@ybiquitous ybiquitous added the status: needs discussion triage needs further discussion label May 16, 2023
@romainmenke
Copy link
Member

romainmenke commented May 16, 2023

Media Feature name validation :

I think all these rules can use the exact same logic as the one we just updated (media-feature-name-no-unknown).

media-feature-name-allowed-list
media-feature-name-case
media-feature-name-disallowed-list
media-feature-name-no-unknown

no-duplicate-at-import-rules :

This does not support cascade layers or support conditions.
Ideally it is refactored to support the latest syntax for @import.

We can use the parser algorithms to separate layer(), supports() and the media queries and then pass the media queries to the media query parser.

But it wasn't clear to me if the media query needed to be parsed for this rule.
If I read the code correctly it is only important to split multiple media queries. (e.g. screen, print becomes ['screen', 'print'])

This can also be done with the parser algorithms.

media-feature-name-value-allowed-list :

This supports non-standard syntax in media feature values.
I don't think we can port this rule.

@media ($foo < $bar) {}

It is impossible to know if $foo is the feature name or the value because the order can be switched.

unit-disallowed-list :

Could be done with purely CSS Tokens.

unit-no-unknown :

I am unsure why there is such special handling of x here.
I've seen some recent commits related to this, but it isn't clear why x isn't just added as a known unit.

This rule doesn't validate the context for any other unit.

transition-duration: 10px, width: 10khz, ...

Otherwise no issues with porting this one.

@ybiquitous
Copy link
Member Author

ybiquitous commented May 17, 2023

@romainmenke Thank you so much for the first investigation! I list up tasks below:

(Edit: moved list to top).

@ybiquitous ybiquitous added status: ready to implement is ready to be worked on by someone type: refactor and removed status: needs discussion triage needs further discussion labels May 17, 2023
@ybiquitous
Copy link
Member Author

I've labeled the issue as ready to implement. We hope for one PR per rule. Please consider contributing if you have time.

@jeddy3
Copy link
Member

jeddy3 commented Jun 29, 2023

I've moved the list to the top to create a task list.

media-feature-name-value-allowed-list: This supports non-standard syntax in media feature values.

We don't typically support non-standard syntax in the built-in rules. It's slipped in as a mistake. As it's undocumented, I believe we can do away with it as part of the refactor. We may also want to patch up the non-spec tests and README examples, e.g. @media screen and (768px < min-width) {} should be width.

@romainmenke
Copy link
Member

romainmenke commented Jun 29, 2023

We may also want to patch up the non-spec tests and README examples, e.g. @media screen and (768px < min-width) {} should be width.

Currently the difference between width and min-width is used to illustrate that this rule considers these as separate features and doesn't unprefix min-width to width.

or at least that is how I interpreted these tests :)

Even if incorrect for the CSS specification when used in a range context I still think this is a valuable example.

I propose we leave that as is for now, but maybe worth it to move this to a separate issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone
Development

Successfully merging a pull request may close this issue.

3 participants