feat(atomic): add support for atomic using styled API

[juanferreras]

Motivation

This continues upon the excellent work by @jpnelson on adding first class (opt-in) Atomic CSS support onto Linaria. The motivation (and community support) can be seen on the original issue #225

Summary

This PR attempts to use the same patterns in #867 :

• adds a styled export on @linaria/atomic which simply re-exports the one in @linaria/react, but can be used to make atomic-styled opt-in
• when using import { styled } from '@linaria/atomic', all the CSS rules within are extracted as atomic classes and re-used across different components
• all the 'static' CSS (either plain static or interpolations that can be evaluated in build time) are extracted as atomic classes
• dynamic interpolations are converted to unique CSS custom properties based on the exact source code, and then atomized, meaning that different components using the exact same dynamic interpolation will end up re-using the same atomic class.
• using const Component = styled(AnotherComponent) should still work as expected (and it should be compatible with @linaria/react too). There might be certain edge cases with property priorities (wrapping components currently boosts atomic property priorities by 1, which should be enough in most cases to override AnotherComponent original atom classes).
• as introduced by feat(core): allow renaming of css template literals #973 , you can also rename the local import, such as import { styled as atomicStyled } from '@linaria/atomic' and it should still work as expected.

Examples

Input

import { styled } from '@linaria/atomic';

const Component = styled.div`
  color: blue;
  margin: ${100 / 2}px;
  background-color: ${props => props.color};
`;

Output

import { styled } from '@linaria/atomic';
const Component = /*#__PURE__*/styled(\\"div\\")({
  name: \\"Component\\",
  vars: {
    \\"1p69eoh\\": [props => props.color]
  },
  class: \\"atm_7l_13q2bts atm_e2_12xxubj atm_gi_12am3vd atm_2d_15g95l5 c17gu1mi\\",
  atomic: true
});

// CSS Output
/*
CSS:
.atm_7l_13q2bts{color:blue;}
.atm_e2_12xxubj{height:100px;}
.atm_gi_12am3vd{margin:50px;}
.atm_2d_15g95l5.atm_2d_15g95l5{background-color:var(--1p69eoh);}
*/

Test plan

Several tests were added with the behaviour described above, and all existing tests were also passing correctly locally (EDIT: not sure why CI v10 can't resolve the internal dependency, locally seems to work fine and the other CI started passing too). Please check the snapshot generated to confirm the functionality works as you'd expect.

I've also applied this changes on the /website and it looks like there's no visual difference when replacing

- import { styled } from '@linaria/react'
+ import { styled } from '@linaria/atomic'

See the following video
https://user-images.githubusercontent.com/8507996/163470135-77dccb6d-82c2-49fc-b161-ae9de9435326.mp4

[Anber]

Hi @juanferreras!
Thank you for such a great job!

1. I don't see any problems with atomizing dynamic props. Do you? It will be something like this and it should work:

.atm_7l_13q2bts{color:blue;}
.atm_gi_12am3vd{margin:50px;}
.atm_f4_c17gu1mi {background-color: var(--f4_c17gu1mi);}

2. Do you mean, when the wrapped component is also atomic? It shouldn't be a problem since the target component has its own list of classes and the result component can just concat with it. Even interop between atomic and classic should have work.
3. Yes, we need it. A styled component can be used as a selector and in that case, we need a class name, even if there is no associated styles at all.
4. The same reason as 3.

[jpnelson]

Hi @juanferreras , thanks so much for working on this! This is really exciting to see, I'm glad it's been useful!

My thoughts on your questions:

1. I agree with @Anber , I think it could make sense to atomize those classes too. It'd be a little redundant since the intent is that these atoms never get reused (the css variable is unique to the component, by design), but it might make things simpler to reason about. It might also help with the answer to 2:
2. I think it is very close to working with atomic wrappers too: since styled is using cx, I think all you'd need to do is swap the ordering here:

// Note: order of arguments to cx swapped so that extending comes after extended
filteredProps.className = cx(
        options.class,
        filteredProps.className || className
      );

I think this shouldn't change anything other than the class order for non-atomic styled, so would be ok to do – otherwise we could change the behavior just for @linaria/atomic's version of styled, but it seems unnecessary.

From my limited testing, it seems like it works if you swap these lines and remove the gating in getTemplateType

Questions 3 and 4: It sounds like if we go with the suggestion in 1 (atomize even dynamic styles) that this class would always be empty, but it'd probably be easiest with the existing infrastructure to have this class be generated and applied to elements. If people don't want that, they can always just use atomic css directly.

Thanks for your work on this and sorry for the delayed review! I'm very enthusiastic on the idea, if I can provide any assistance on getting tests or functionality to work, let me know! 🙏

[callstack-bot]

Hey @juanferreras, thank you for your pull request 🤗.
The coverage report for this branch can be viewed here.

[juanferreras]

thanks so much to both on the feedback! Updated the PR and the tests with the feedback so far.

1. dynamic interpolations are now also getting atomized.
   The current version still maintains the existing var(--className-index) variables which are unique to each styled usage, not sure this was a request for change @Anber (happy to give it a try if so!)

- .atm_f4_c17gu1mi {background-color: var(--f4_c17gu1mi);}
+ .atm_f4_c17gu1mi {background-color: var(--cccybe9-1);}

Not sure how feasible would it be to create an atom based on the exact interpolation text (eg. property: props => props.backgroundColor instead of creating it based on the text with property: var(--slug-i) already), but I fear actually merging the custom properties names could change existing behaviour (I can think of edge cases using inherit, not sure how common in practice)

---

2. styled(Component) is now also atomized

it works pretty much seamlessly with the extra change you've sent @jpnelson! There's one test case that changed its behaviour styled.test.js > provides linaria component className for composition as last item in props.className, will look into it.

Interop between styled react/atomic seems to work great too, but there's one caveat. Given how property priorities are implemented (eg. .atm_xx_yy.atm_xx_yy as the way by which background-color has bigger priority/specificity than background), atomic over atomic compositing might fail to override certain rules if it doesn't equal/beat property priority, for example:

const TitleAtomic = atomicStyled.h1`background-color: blue`;
const TitleReactOverAtomic = styled(TitleAtomic)`background: red`; // shows red
const TitleAtomicOverAtomic = atomicStyled(TitleAtomic)`background: red`; // would show blue

I've pushed an update to increase propertyPriority by 1 when using styled(OtherComponent) from @linaria/atomic, but @jpnelson let me know if you think we shouldn't do this and instead simply document this as a caveat in ATOMIC_CSS.md

---

P/S: it looks like CI might be caching dependencies incorrectly – as without changes it's now passing OK in 1 of the checks (not on node v10 still from the same error as before)

P/S 2: @jpnelson as per your comment, more than happy to update this PR once you land #973 and apply the changes on my end – many thanks for the work there too! super useful 🙏

[Anber]

1. I'm also not sure if it is safe, but at least for atomic, variable names based on property names seams more logical to me. I don't think breaking change for existing behaviour is a problem since we do not have a stable release yet. However, I would like to hear @jpnelson's opinion.

[juanferreras]

1. I'm also not sure if it is safe, but at least for atomic, variable names based on property names seams more logical to me. I don't think breaking change for existing behaviour is a problem since we do not have a stable release yet. However, I would like to hear @jpnelson's opinion.

@Anber sounds like a great idea! I'll update this PR with the latest and see if I can generate any var(--hash) where the hash can be unique to the exact interpolation text.

I'll start experimenting with something like slugify(ex.getSource() || generator(ex.node).code), where ideally:

const Component1 = styled.div`
  /* compiles to `.atm_aa_xx` */
  color: ${props => props.color}; 
`;
const Component2 = styled.div`
  /* compiles to the same `.atm_aa_xx` */
  color: ${props => props.color}; 
  /* same `var(--hash)` but different property, different atom class `.atm_bb_xx` */
  border-color: ${props => props.color}; 
  /* different `var(--hash)` and different property, different atom class `.atm_cc_yy` */
  background-color: ${props => props.color || 'white'}; 
`;

[juanferreras]

@Anber / @jpnelson moving this to ready for review! many thanks for the support thus far.

I'm sadly still struggling with tests on CI (they don't like the re-export of styled from packages/atomic, which somehow locally works without any issues). Any tips on how to debug / iron this issue?

For a small recap of updates,

1. dynamic properties are hashed based on the exact interpolation source code before being atomized.
2. styled wrapping works and boosts property priority of atoms by 1.
3. import { styled as atomicStyled } should now also work.
4. the classlist reordering change in styled.ts was made only for atomic-styled to prevent breaking existing behaviour (which 1 test in packages/react is very explicit upon)

Thanks!

[Anber]

Great work!

[juanferreras]

Great work!

excellent! thanks for approving and merging @Anber! I'll keep an eye out when it gets released as a next version beta to continue testing with real production use cases

[jpnelson]

+1 Great work! Very exciting progress on this!

I've pushed an update to increase propertyPriority by 1 when using styled(OtherComponent) from @linaria/atomic, but @jpnelson let me know if you think we shouldn't do this and instead simply document this as a caveat in ATOMIC_CSS.md

This sounds preferable to me, better to not have people worry about that. We'll just need to keep in mind that hasPriority should ensure that it always beats other property priorities to avoid the issue again. That would only come up if we needed more than 2 levels of regular priority (beyond longhand / shorthands) and I don't really see how we would.

I'm also not sure if it is safe, but at least for atomic, variable names based on property names seams more logical to me. I don't think breaking change for existing behaviour is a problem since we do not have a stable release yet. However, I would like to hear @jpnelson's opinion.

This makes sense to me – a declaration is the property + the value, if we're providing the value though via a css variable, we can just hash the property – any collisions there would be intentional. It might be interesting to write a test for the collision case just to see the output (a dynamic property component overriding another dynamic property of the same type)

As for the CI, I was running the tests locally too, and I was able to actually reproduce the error a couple of times. The error seemed flaky though – rerunning locally fixed the issue, and I wasn't able to consistently reproduce it

circleci has a local execution option that I was trying to get it to work. To use that, I first had to modify the config locally as it doesn't support multiple parallel workflows:

index 804af9a1..6619b7af 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -90,6 +90,7 @@ jobs:
   unit-tests-node-v10:
     executor: node-v10
     steps:
+      - install-dependencies
       - unit-tests
 workflows:
   multiple_builds:

With this, the problem is actually reproducible locally – just a bit slow to run (I think it wouldn't have any of the caching as it does in CI)

(I also needed to follow this workaround for the M1 mac to get circleci local to work)

[Anber]

It looks like error. Should it be atomicStyled instead of reactStyled?

[Anber]

If so, I've already fixed this :)

[juanferreras]

@Anber good catch! sorry for missing it!

https://github.com/callstack/linaria/blob/master/packages/babel/src/evaluators/templateProcessor.ts#L331-L339

I understand this should have been replaced to use either localName?.atomicStyled or localName?.styled depending on whether styled.type === 'atomic-styled'or not.

LMK if you'd like me to open a follow up PR with the fix – many thanks!

[juanferreras]

+1 Great work! Very exciting progress on this!

I've pushed an update to increase propertyPriority by 1 when using styled(OtherComponent) from @linaria/atomic, but @jpnelson let me know if you think we shouldn't do this and instead simply document this as a caveat in ATOMIC_CSS.md

This sounds preferable to me, better to not have people worry about that. We'll just need to keep in mind that hasPriority should ensure that it always beats other property priorities to avoid the issue again. That would only come up if we needed more than 2 levels of regular priority (beyond longhand / shorthands) and I don't really see how we would.

I'm also not sure if it is safe, but at least for atomic, variable names based on property names seams more logical to me. I don't think breaking change for existing behaviour is a problem since we do not have a stable release yet. However, I would like to hear @jpnelson's opinion.

This makes sense to me – a declaration is the property + the value, if we're providing the value though via a css variable, we can just hash the property – any collisions there would be intentional. It might be interesting to write a test for the collision case just to see the output (a dynamic property component overriding another dynamic property of the same type)

As for the CI, I was running the tests locally too, and I was able to actually reproduce the error a couple of times. The error seemed flaky though – rerunning locally fixed the issue, and I wasn't able to consistently reproduce it

circleci has a local execution option that I was trying to get it to work. To use that, I first had to modify the config locally as it doesn't support multiple parallel workflows:

index 804af9a1..6619b7af 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -90,6 +90,7 @@ jobs:
   unit-tests-node-v10:
     executor: node-v10
     steps:
+      - install-dependencies
       - unit-tests
 workflows:
   multiple_builds:

With this, the problem is actually reproducible locally – just a bit slow to run (I think it wouldn't have any of the caching as it does in CI)

(I also needed to follow this workaround for the M1 mac to get circleci local to work)

thanks for all the details @jpnelson! I've misunderstood on my end that with property names we were talking about prop names as in, React prop names used inside the interpolation, and not the actual CSS properties of the declarations.

This is pretty much the only downside I can see towards making all interpolations just atomize("color: var(--color)") (if I understand correctly now). Given how CSS variable inheritance works, using the keyword inherit could be quite problematic.

1. Currently on @linaria/react this happens when nesting the same component over itself
2. As of this PR on @linaria/atomic this would happen when nesting the exact same CSS property+interpolation code
3. If we change that to just color: var(--color), the collisions could become really common.

To be fair, we could just document that inherit means inherit from last defined CSS variable and not from parent CSS applied (I don't know if there's any other problematic keyword).

P/S: I hope I didn't break CI now that's merged 😬 – I wonder if there's any place I've missed adding the new dependency on