feat(atomic): create an atomic package for the css API#867
feat(atomic): create an atomic package for the css API#867Anber merged 2 commits intocallstack:masterfrom
Conversation
| displayName: displayName!, | ||
| start: path.parent?.loc?.start ?? null, | ||
| }; | ||
| path.replaceWith(atomicClassObject); |
There was a problem hiding this comment.
Another thought I had was that this is different from the css API in that the atomic css template literal returns an object and requires you to use the atomic cx function to convert that into styles (by resolving the merging of styles).
Some alternatives:
We could instead produce a string that the cx function would need to resolve, like this:
path.replaceWith('atm_prophash1_valuehash1 atm_prophash2_valuehash2')
and then the atomic cx function would know to do the merging based on the structure of the class names.
In performance testing on https://jsbench.me/, there's a slight disadvantage to doing the string manipulation, but only slight.
I think this is probably not preferable, as using the class names as strings without cx from @linaria/atomic will be a gotcha (you'd end up with conflicts resolved by specificity, which will be confusing). The object approach should also be type safe when using css from @linaria/atomic
Interested in your thoughts though
There was a problem hiding this comment.
I think, there should be just one cx because classes for it can come from different libs and modules, some of them can use core whereas another can use atomic.
packages/atomic/src/cx.ts
Outdated
| ): string; | ||
| } | ||
|
|
||
| const cx: ICX = function cx(...rest) { |
There was a problem hiding this comment.
We already have an implementation of cx in @linaria/core. Probably, we can move it to the new package (eg. @linaria/utils), modify it in order to support new fetures and re-export it in core and atomic.
packages/babel/yarn.lock
Outdated
| @@ -0,0 +1,321 @@ | |||
| # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
There was a problem hiding this comment.
Shouldn't be here. Probably, just added by a mistake.
| @@ -0,0 +1,45 @@ | |||
| import postcss from 'postcss'; | |||
There was a problem hiding this comment.
I don't think that such a big dependency is a good thing, especially if it is used only by a fraction of users. It would be ok if it was a dependency of atomic but we have to think about how such inversion can be implemented.
There was a problem hiding this comment.
Yeah, that's fair. A couple of ideas how to do this:
- Use an optional peer dependency (might be confusing for people who are using NPM pre v7 though, where it would just be a peer dependency)
- Invert the dependency so that
@linaria/atomichas theatomizefunction, and the postcss dependency. The atomize function can be a part of the linaria config used by the babel preset, and if you want to use atomic styles, pass inatomize: require('@linaria/atomic').atomizein the config
I'll go with 2 for now, I would be open to trying 1 too though!
| atomicRules.forEach((rule) => { | ||
| state.rules[`.${rule.className}`] = { | ||
| cssText: rule.cssText, | ||
| start: path.parent?.loc?.start ?? null, |
There was a problem hiding this comment.
How do you think, is it possible to specify the real positions of rules for source-maps?
There was a problem hiding this comment.
It's a good question – I think it's a little tricky because a single atom might have come from multiple different modules.
The way it's done here, it should be that an atom's source map points to the last place it was generated from (it does work correctly on the website example). I think that's probably better than nothing – in the case where you have a unique atom, and you want to know where it's coming from, the source map will work for that. In the case where the atom is shared, the source map will point to at least one place where it is defined.
| displayName: displayName!, | ||
| start: path.parent?.loc?.start ?? null, | ||
| }; | ||
| path.replaceWith(atomicClassObject); |
There was a problem hiding this comment.
I think, there should be just one cx because classes for it can come from different libs and modules, some of them can use core whereas another can use atomic.
84ca239 to
be770f8
Compare
|
Thank you for your feedback @Anber ! I've updated the PR with the suggestions, primarily:
I'd love to hear what you think – thanks again for the feedback |
|
Hi @Anber , did you have any further thoughts on this? I'd be happy to help address any more outstanding issues 🙏 |
|
Hi @jpnelson! It looks good! I'm going to merge it and release it with the next beta. |
Motivation
This is an implementation of #225 – more details for the motivation exist there.
Summary
This introduces a new package – @linaria/atomic – which has a different export for
css. It has roughly the same API, but creates atomic styles instead.It also includes a new utility,
cx, which can be used to merge together these atomic styles.Examples
Input
Output
Nesting and cascading
Note: this new API by nature does not support nesting or cascading. For example, the following won't work in atomic css:
Instead, there are other options (use
cssfrom@linaria/core, or apply the styles atomically to thea)For this reason, I think it's worth keeping
@linaria/coreseparate, and having@linaria/atomicbe available if people would like to use the atomic version, that comes with this limitation.Note that I have kept
styledout of this PR to keep this incremental, but I think we can addstyledatomic support in a very similar way. I tried this successfully in a proof of concept, but we'd need to clean that up a bit. I'd be happy to create a separate PR for that.Follow up work
I think this is the smallest usable API that we could create, but there are a couple of things missing from this PR that would be helpful:
styledAPI equivalent of this@linaria/serverto extract atomic css the same way critical CSS is extractedcxcxat build time too to simplify the runtime costs in a lot of cases. Note that we can't always do this, so this is just a performance optimizationImplementation
While the API to access it exists in
packages/atomic, the hard work is almost entirely done inpackages/babel. The changes are to:isStyledOrCssto begetTemplateType, which can now be one ofcss | styled | atomic-css(we can addatomic-styledwhen we support thatatomic-cssas we would withcssin terms of processing the template literal and evaluating, with some exceptions:csstemplate literal not with just a class name, but with an object that has keys (See here). This is necessary so thatcxcan merge together the atomic class names (the keys are the CSS properties for the class name).I created this as the smallest complete PR I could for this, let me know if you'd like it to be split up further.
Test plan
I've added some unit tests, snapshots, and converted one of the components on the docs site to use atomic css.