Stylis V4 compatibility

[tpict]

Adds compatibility for Stylis V4.

I've added an explicit check for @keyframes. Are there any other at-rules whose properties should be exempt from extra scoping?

[Andarist]

Thank you for the PR - I'll try to review this somewhat soon, but truth to be told this requires some focus and this is not on the top of my priority list so it might take me some time to get back to it.

[tpict]

This implementation causes weird issues with the & selector when used in conjunction with Emotion 11. After updating the scope of a node, child nodes that use & already have the new scope in their props, e.g. when traversing

.some-class {
  &.something-else {
   ...

the parent node is modified to #my-extra-scope .some-class, and when the child node is visited it's already been expanded to #my-extra-scope .some-class.something-else, so this code adds a duplicate #my-extra-scope selector.

I tried to remedy this by keeping track of modified parent nodes in a WeakSet and skipping their descendants, but not all selectors are expanded when they're visited, so they wind up without any extra scoping at all. Furthermore, the expansion doesn't seem to happen with Stylis alone, only when using Emotion, so I'm not even sure how to test for this.

[Andarist]

I would literally set up tests with Emotion here to test this. I'm not sure why Emotion would make it to affect children. Nodes are actually generated by Stylis and while some object references are shared within the "tree" the generated rules should not be affected like this. Maybe it's the compat plugin's fault? Nothing comes to my mind that would affect this for the case you have presented but it seems like the most likely culprit.

[tpict]

Thanks for the lead! Placing this plugin ahead of compat does fix the issue.

Looking at the Stylis source code, it appears that each node passes through each plugin before moving on to the next node. The compat plugin is combining the current selector onto its parent, which already has the extra scope, hence the duplication. I guess I need to implement the opposite of the logic in compat to skip nodes that have already been joined to their parent.

[hasanavi]

The latest version of @emotion/react doesn't work from the master branch - is this PR going to be merged soon?

[Andarist]

Could you explain the logic behind those conditions here? writing Stylis plugins is tricky and I would like to recheck the thought process behind this - I'm not saying there is anything wrong with it, it would just make it easier for me to review it

[tpict]

I'm no longer working on the project that I wrote this for so I can't test out exactly what CSS elements this is targeting, but I do know that it's targeting elements that pass through the compat plugin unchanged. Without this logic, the extra scope would be applied to nested selectors and pseudo-selectors before being joined to parent selectors by compat, resulting in the "joined" selector including repeated references to the extra scope.

[luzannew]

I can confirm this PR is working fine together with styled-components v5.2.1 which is using v4 of stylis.

[Andarist]

Styled Components v5 is not using Stylis v4 yet: https://github.com/styled-components/styled-components/blob/v5.2.1/packages/styled-components/package.json#L69

[luzannew]

Oh strange. looks like on the master branch they are using it: https://github.com/styled-components/styled-components/blob/master/packages/styled-components/package.json#L73

So I'm not sure from which branch they released v5.2.1

[rr-justin-hanselman]

Hello,

I was just wondering what the blockers are for this plugin fix? From the comments, the problems seem to be resolved?

[Andarist]

the main blocker is my lack of time to focus on this one

[rr-justin-hanselman]

Is there anything more external users (like myself) can do to help you? Or is just a matter of the repo owner finishing the PR vetting + release?

For now, I've just copied the potential changes and am using them in a local lib file, but I would love to seem them as part of the normal npm registry!

[Andarist]

If you could recheck the implementation - preferably with some notes on why it is written in a way that it is written - this would help a lot. I suspect that this version might not work properly with nested at rules but I have also not checked it.

In general, I'm not confident with merging this without understanding the thought process behind this implementation. I'm not saying that it's incorrect or wrong - it's just that Stylis plugins are quite tricky to get right.

Preparing PR that would merge master to this branch would also be helpful as this has some conflicts right now.

[evantd]

Re:

Nodes are actually generated by Stylis and while some object references are shared within the "tree" the generated rules should not be affected like this. Maybe it's the compat plugin's fault?

This comment in the compat plugin seems suspicious:

  // if this is an implicitly inserted rule (the one eagerly inserted at the each new nested level)
  // then the props has already been manipulated beforehand as they that array is shared between it and its "rule parent"

[nelsondude]

Is there any update on the status of this PR? Would be great to get this plugin updated.

[iampava]

Any updates on this?

[tpict]

Going to close this out in favour of #14