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

Fix report() for index/endIndex zero values #7565

Merged

Conversation

romainmenke
Copy link
Member

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

None, but related to #7518 (comment)

Is there anything in the PR that needs further explanation?

a {}

This stylesheet has a selector with a length 1 at the start of a stylesheet.
This is passed to PostCSS as index: 0 and endIndex: 0 since endIndex is exclusive.

However both Stylelint and PostCSS have boolean checks for endIndex instead of a check that it is a number.

So the warning is reported as if no endIndex was set.
This does a fallback to the entire length of the node.

a {}
^  ^

PostCSS has already been fixed but awaiting release : postcss/postcss#1932


This should have a unit test, but I noticed that there aren't any unit tests for report.mjs with endIndex.

So I wasn't sure on what the best and most succinct test would be here.

Copy link

changeset-bot bot commented Mar 20, 2024

🦋 Changeset detected

Latest commit: 847f8f9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@romainmenke Thanks for the pull request! In addition to endIndex, does also checking index make sense?

-	} else if (index) {
+	} else if (typeof index === 'number') {

} else if (index) {

@romainmenke
Copy link
Member Author

Tested with also a check for index and that seems safe to me, so I've also added this.

I don't think it makes a practical difference because in the absence of a start index there already are fallbacks to the start in PostCSS, so "not specified" is always equal to 0.

But I like the consistency :)


I've also updated PostCSS so that the fix is complete.

@ybiquitous ybiquitous changed the title Fix end positions for CSS entities with a length of 1 at the start of a stylesheet Fix report() for index/endIndex zero values Mar 21, 2024
Copy link
Member

@ybiquitous ybiquitous 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. LGTM 👍🏼

Besides, can you add a changelog item according to this PR title?

Fixed: `report()` for `index`/`endIndex` zero values

After adding the item, feel free to merge 👍🏼

@romainmenke
Copy link
Member Author

Thank you for the feedback @ybiquitous 🙇

@romainmenke romainmenke merged commit dba9a6e into main Mar 21, 2024
14 checks passed
@romainmenke romainmenke deleted the allow-endIndex-to-be-zero--generous-caiman-66321db509 branch March 21, 2024 08:50
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

2 participants