Fix inaccurate end positions in `selector-max-type` (breaking) #7509

romainmenke · 2024-02-01T18:12:34Z

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

Is there anything in the PR that needs further explanation?

No, it's self-explanatory.

changeset-bot · 2024-02-01T18:12:37Z

⚠️ No Changeset found

Latest commit: 8819c54

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

romainmenke · 2024-02-01T18:16:47Z

lib/rules/selector-max-type/__tests__/index.mjs

-		{
-			code: '.foo { [&] {} }',
-		},


[&] can not be parsed and gives a warning, but [&] also isn't a valid selector.

I would prefer to just drop this test because bogus inputs will always give bogus outputs.

Editted to clarify that this comment is only about [&]

Don't know if our policies have changed since

Non-standard selectors should be ignored, and tests for them should be accepted.

but shouldn't it fail gracefully?

Are non-standard selectors all selectors that aren't standard, or are they only those that are valid in sass, less,...

Nuance between :

completely bogus

valid in a CSS flavor, but not in standard CSS

I don't know but I think that we should at least communicate with stylelint-scss so that they can prepare a rule synchronised with our next release to offer as a drop-in solution to annoyed users.
That way you may drop anything that isn't standard.

I'm sorry, I think there is some confusion here :)

[&] is not a sass or less selector.

It isn't a valid selector anywhere as far as I know.
If it is valid anywhere, please let me know because then my entire premise is incorrect.

selector-max-type already used and continues to use isStandardSyntaxSelector to ignore any selector that is detectable as sass, less, ...

That [&] was fine in this rule was incorrect, likely a result of postcss-resolve-nested-selector doing find/replace with strings.

It is also unlikely that users are writing [&] because this gives errors in general, even when it passes for this rule : https://stylelint.io/demo/#N4Igxg9gJgpiBcIB0AzCEAEwMB0B2GGA2gGQC6W+hhkANhAE7wYDmDMMeA3FRgL74+IADQgUAS1owAcgEMAtnEQwAHgoAOUpGADOOkeAh4JLBCGC8cIVQBdOUHVeZFehKzpsBPKbXF4bALSQxuIsAR6yeFCyDFBWvGSCBsEmAGKM8rI2ZgBWOkYGsOr6iBYEuCAe3jC+-k4VtFkwHlbClpVePn6BKaHhNpHRsfVWjXYtIEl8QA

Editted to clarify that this comment is only about [&]

Sorry my bad, I was commenting on the basis that it was about the lines removed from line 36 to 44 which contained code: '.foo { &__bar {} }',.
[&] is probably "completely bogus", you are right.

romainmenke · 2024-02-01T18:18:57Z

lib/rules/selector-max-type/__tests__/index.mjs

+		{
+			code: '.foo { &-bar {} }',
+			description: '&-bar is an invalid selector, types must precede `&`',
+			message: messages.expected('&-bar', 0),
+			line: 1,
+			column: 8,
+			endLine: 1,
+			endColumn: 13,
+		},
+		{
+			code: '.foo { &__bar {} }',
+			description: '&__bar is an invalid selector, types must precede `&`',
+			message: messages.expected('&__bar', 0),
+			line: 1,
+			column: 8,
+			endLine: 1,
+			endColumn: 14,
+		},


This is a breaking change.

If people use this rule while writing sass style nesting they will get errors after updating. Given the popularity of sass style nesting it will definitely be noticed when we change this.

Maybe add a specific if to detect the pattern and console.warn a hint?

Or should we add detection for this in isStandardSyntaxSelector?

&__bar and &-bar are never standard selectors, they can only be sass, less, ...
So I think isStandardSyntaxSelector should return false in this case.

I think this might be the least intrusive way forward.

are never standard selectors

If true, it ought to be fixed in a separate commit.
IIRC this util is being exported so it might have consequences.
We shouldn't care though, utils ought to match their names.

I agree with adding detection to isStandardSyntaxSelector by another pull request, as @Mouvedia suggested.

I agree with the refactoring using the nice package "@csstools/selector-resolve-nested unless it introduces breaking changes.

I am a bit confused now :)
I think that any refactor using this package will have some amount of breaking changes and should be saved for a next major.

I think we have these options:

do not use this package at this time, save it for a next major. We close these PR's and focus purely on the issue of inaccurate end positions which might also have a different solution.

Start work early on the next major and start refactoring all selector rules to use this package. How exactly we work on the next major, this early, is a different topic :)

A mix of 1 and 2 where we fix inaccurate end positions in a way that facilitates a future switch to this package. This also implies having a good understanding of this package and how it will integrate in the future.

Option 1 might not find a solution and it can not solve issues with nesting. But it could be a non-breaking step forward.

Notes :

I am not attached to these changes, the proposed package, ... These can all be discarded if they are not the right way forward.

I am fine with shelving this work for a few months. Closing these PR's and reviving the effort later.

Thank you for the detailed explanation. I misunderstood there was room to refactor via @csstools/selector-resolve-nested somewhere in the code base. 👍🏼

We just released the current major version (16.0.0) a few months ago (Dec 2023), so I predict the next major release will be several months later. I guess staring the work for the next major version is a bit too early at this point.

Of course, I welcome non-breaking changes. Also, since end-position bugs are a bit less critical, we could address other works, e.g. considering the migration path from postcss-resolve-nested-selector to @csstools/selector-resolve-nested.

For the record, I vote 3.
It might be annoying to keep both packages and try to navigate/arbitrate breaking changes but it will result in a smoother transition.

I have a vague idea of what 3 looks like.
I will try to prepare a PR with that shortly.

Then this is less abstract :)

see : #7518

Mouvedia · 2024-02-01T19:16:11Z

I am not seeing selector-max-type under https://github.com/stylelint-scss/stylelint-scss/tree/master/src/rules, does that mean it will have to be turned off if you are using sass?
e.g. &__foo {}

…g-bec59ebff0

romainmenke · 2024-03-17T17:57:54Z

Closing this, but also stored the interesting bits in a gist here : https://gist.github.com/romainmenke/32b8e196d0bde94b4429710e3c6ada38

romainmenke force-pushed the selector-end-positions--unassuming-marsh-frog-bec59ebff0 branch from 7aba548 to 8897a3a Compare February 1, 2024 18:15

romainmenke changed the title ~~Selector end positions unassuming marsh frog bec59ebff0~~ Fix inaccurate end positions in selector-max-type Feb 1, 2024

romainmenke force-pushed the selector-end-positions--unassuming-marsh-frog-bec59ebff0 branch from 8897a3a to 44bab90 Compare February 1, 2024 18:20

romainmenke commented Feb 1, 2024

View reviewed changes

Fix inaccurate end positions in selector-max-type

2d633cc

romainmenke force-pushed the selector-end-positions--unassuming-marsh-frog-bec59ebff0 branch from 44bab90 to 2d633cc Compare February 1, 2024 18:22

increase test coverage for end positions

c9d056e

romainmenke mentioned this pull request Feb 1, 2024

Fix inaccurate end positions when postcss-resolve-nested-selector is used in selector-* #6234

Open

13 tasks

romainmenke and others added 4 commits February 1, 2024 23:17

cleanup and more test coverage

5392d76

cleanup

8749a17

Merge branch 'main' into selector-end-positions--unassuming-marsh-fro…

074e16c

…g-bec59ebff0

stop iteration early

206c330

romainmenke mentioned this pull request Feb 6, 2024

Fix selector-max-type end positions #7518

Merged

romainmenke changed the title ~~Fix inaccurate end positions in selector-max-type~~ Fix inaccurate end positions in selector-max-type (breaking) Feb 6, 2024

rename

8819c54

romainmenke closed this Mar 17, 2024

romainmenke deleted the selector-end-positions--unassuming-marsh-frog-bec59ebff0 branch March 17, 2024 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix inaccurate end positions in `selector-max-type` (breaking) #7509

Fix inaccurate end positions in `selector-max-type` (breaking) #7509

romainmenke commented Feb 1, 2024

changeset-bot bot commented Feb 1, 2024 •

edited

romainmenke Feb 1, 2024 •

edited

Mouvedia Feb 1, 2024

romainmenke Feb 1, 2024

Mouvedia Feb 1, 2024

romainmenke Feb 1, 2024 •

edited

Mouvedia Feb 2, 2024 •

edited

romainmenke Feb 1, 2024

Mouvedia Feb 1, 2024

romainmenke Feb 1, 2024

Mouvedia Feb 2, 2024

ybiquitous Feb 5, 2024

romainmenke Feb 6, 2024

ybiquitous Feb 6, 2024

Mouvedia Feb 6, 2024 •

edited

romainmenke Feb 6, 2024

romainmenke Feb 6, 2024

Mouvedia commented Feb 1, 2024 •

edited

romainmenke commented Mar 17, 2024

Fix inaccurate end positions in selector-max-type (breaking) #7509

Fix inaccurate end positions in selector-max-type (breaking) #7509

Conversation

romainmenke commented Feb 1, 2024

changeset-bot bot commented Feb 1, 2024 • edited

⚠️ No Changeset found

romainmenke Feb 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romainmenke Feb 1, 2024 • edited

Choose a reason for hiding this comment

Mouvedia Feb 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mouvedia Feb 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mouvedia commented Feb 1, 2024 • edited

romainmenke commented Mar 17, 2024

Fix inaccurate end positions in `selector-max-type` (breaking) #7509

Fix inaccurate end positions in `selector-max-type` (breaking) #7509

changeset-bot bot commented Feb 1, 2024 •

edited

romainmenke Feb 1, 2024 •

edited

romainmenke Feb 1, 2024 •

edited

Mouvedia Feb 2, 2024 •

edited

Mouvedia Feb 6, 2024 •

edited

Mouvedia commented Feb 1, 2024 •

edited