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

Update selector parser #4595

Merged
merged 3 commits into from
Feb 13, 2020
Merged

Update selector parser #4595

merged 3 commits into from
Feb 13, 2020

Conversation

jeddy3
Copy link
Member

@jeddy3 jeddy3 commented Feb 13, 2020

Which issue, if any, is this issue related to?

References https://github.com/orgs/stylelint/teams/core/discussions/18

Replaces #3988

Is there anything in the PR that needs further explanation?

I've ported over the changes from #3988. We changed our formatting and moved to TS JS Docs from flow since we made the original pull request

I believe only two rules are affected by this update:

  • selector-descendant-combinator-no-non-space will now outright ignore selectors containing comments
  • selector-pseudo-class-parentheses-space-inside can now longer autofix pseudo-classes that contain comments

I think these are minor limitations for the sake of updating. I've added a note to the READMEs of both these rules.

We have fixed some other bugs by updating the selector parser, e.g. the one in selector-attribute-brackets-space-inside/__tests__/index.js. Things probably balance out in the end.

I've skipped the affected tests.


It'd be great if we could get #4592 and #4591 approved and merged too so that the new workaround feature and related documentation changes are also released.

Copy link
Member

@vankop vankop left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Thanks, @jeddy3!

Could you take a look at few tiny requests, please?

@@ -10,23 +10,5 @@ module.exports = function(node) {
// Ghost descendant combinators around reference combinators like `/deep/`
// postcss-selector-parser parsers references combinators as tag selectors surrounded
Copy link
Member

Choose a reason for hiding this comment

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

This comment shall be changed to match new parser.

Comment on lines 84 to 85
const needsCorrectEscape = attributeNode.value.indexOf(correctQuote) !== -1;
const needsOtherEscape = attributeNode.value.indexOf(erroneousQuote) !== -1;
Copy link
Member

Choose a reason for hiding this comment

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

A little bit of readability:

const needsCorrectEscape = attributeNode.value.includes(correctQuote);
const needsOtherEscape = attributeNode.value.includes(erroneousQuote);

Comment on lines 110 to 111
const needsCorrectEscape = attributeNode.value.indexOf(correctQuote) !== -1;
const needsOtherEscape = attributeNode.value.indexOf(erroneousQuote) !== -1;
Copy link
Member

Choose a reason for hiding this comment

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

const needsCorrectEscape = attributeNode.value.includes(correctQuote);
const needsOtherEscape = attributeNode.value.includes(erroneousQuote);

@jeddy3
Copy link
Member Author

jeddy3 commented Feb 13, 2020

Good catches. I've made the changes.

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Thank you!

We shall create an issue with all of these downgrades. To remember to restore functionality when time comes.

@jeddy3 jeddy3 merged commit b72df96 into master Feb 13, 2020
@jeddy3 jeddy3 deleted the update-selector-parser branch February 13, 2020 22:39
@hudochenkov
Copy link
Member

Do you think we should mention in a changelog that some rules were downgraded?

@jeddy3
Copy link
Member Author

jeddy3 commented Feb 13, 2020

  • Security: updated to postcss-selector-parser@6 due to a vulnerability in one of postcss-selector-parser@3 dependencies (#4595).

@jeddy3
Copy link
Member Author

jeddy3 commented Feb 13, 2020

Do you think we should mention in a changelog that some rules were downgraded?

I think so.

@jeddy3
Copy link
Member Author

jeddy3 commented Feb 13, 2020

  • Security: updated to postcss-selector-parser@6 due to a vulnerability in one of postcss-selector-parser@3 dependencies.(#4595). Due to this update:
    • selector-descendant-combinator-no-non-space will ignore selectors containing comments
    • selector-pseudo-class-parentheses-space-inside can't autofix pseudo-classes that contain comments

@jeddy3
Copy link
Member Author

jeddy3 commented Feb 13, 2020

We shall create an issue with all of these downgrades. To remember to restore functionality when time comes.

Done in #4599 & #4600.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants