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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure @apply of a rule inside an AtRule works #6594

Merged
merged 2 commits into from Dec 17, 2021
Merged

Conversation

RobinMalfait
Copy link
Contributor

@RobinMalfait RobinMalfait commented Dec 17, 2021

This PR will ensure that if you are using @apply something and that something happens to be in an AtRule and that AtRule contains an unrelated node that we only apply the related node... 馃サ

Let's imagine you have the following structure:

.foo {
  @apply bar;
}

@supports (a: b) {
  .bar {
    color: blue
  }

  .something-unrelated {}
}

In this case we want to apply .bar but it happens to be in an AtRule node. We clone that node instead of the nested one because we still want that @supports rule to be there once we applied everything.

However it happens to be that the .something-unrelated is also in that same shared @supports atrule. This is not good, and this should not be there. The good part is that this is a clone already and it can be safely removed. The question is how do we know we can remove it. Basically what we can do is match it against the applyCandidate that you want to apply. If it doesn't match the we can safely delete it.

If we didn't do this, then the replaceSelector function would have replaced this with something that didn't exist and therefore it removed the selector altogether. In this specific case it would result in {} instead of .something-unrelated {}

Fixes: #6246

RobinMalfait and others added 2 commits December 17, 2021 18:34
If that atrule happens to contain another rule that is technically
unrelated.

Co-authored-by: Jordan Pittman <jordan@cryptica.me>
@driskull
Copy link

Not sure that this is related, but with the latest 3.0.7 release I'm now seeing this error:

Cannot read properties of undefined (reading 'parent')

Seems like this PR is the only thing that might be related to reading parent. Is that correct?

@RobinMalfait
Copy link
Contributor Author

@driskull hmm can you create a minimal reproduction repo and open a new issue?

// therefore it removed the selector altogether. In this specific
// case it would result in `{}` instead of `.something-unrelated {}`
if (!extractClasses(rule).some((thing) => thing === applyCandidate)) {
rule.remove()

Choose a reason for hiding this comment

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

@RobinMalfait I'm not sure what exactly the issue is. I'll keep investigating.

My best guess is that something else is missing besides just removing the rule. Maybe the parent still has a reference to the rule which would cause it to be undefined?

Choose a reason for hiding this comment

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

If I do the following, it no longer errors but I'm just shooting in the dark here...

rule.before(rule.nodes)
rule.remove();
return;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please open an issue with a reproduction.

Choose a reason for hiding this comment

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

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

Successfully merging this pull request may close these issues.

@apply with @supports produces extra commas
2 participants