-
Notifications
You must be signed in to change notification settings - Fork 414
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(atomic): create an atomic package for the css API #867
Conversation
displayName: displayName!, | ||
start: path.parent?.loc?.start ?? null, | ||
}; | ||
path.replaceWith(atomicClassObject); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jpnelson
I like it! Thank you for such a great job!
packages/atomic/src/cx.ts
Outdated
): string; | ||
} | ||
|
||
const cx: ICX = function cx(...rest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be here. Probably, just added by a mistake.
packages/babel/src/utils/atomize.ts
Outdated
@@ -0,0 +1,45 @@ | |||
import postcss from 'postcss'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/atomic
has theatomize
function, 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').atomize
in 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you think, is it possible to specify the real positions of rules for source-maps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
css
from@linaria/core
, or apply the styles atomically to thea
)For this reason, I think it's worth keeping
@linaria/core
separate, and having@linaria/atomic
be available if people would like to use the atomic version, that comes with this limitation.Note that I have kept
styled
out of this PR to keep this incremental, but I think we can addstyled
atomic 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:
styled
API equivalent of this@linaria/server
to extract atomic css the same way critical CSS is extractedcx
cx
at 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:isStyledOrCss
to begetTemplateType
, which can now be one ofcss | styled | atomic-css
(we can addatomic-styled
when we support thatatomic-css
as we would withcss
in terms of processing the template literal and evaluating, with some exceptions:css
template literal not with just a class name, but with an object that has keys (See here). This is necessary so thatcx
can 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.