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’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

Merged
merged 2 commits into from Nov 29, 2021

Conversation

jpnelson
Copy link
Contributor

@jpnelson jpnelson commented Nov 6, 2021

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

import { css, cx } from '@linaria/atomic';

const bigRed = css`
  font-size: 2em;
  color: red;
`;

const blue = css`
  color: blue;
`;

export const className = cx(bigRed, blue);

Output

// Expected Output
const className = 'atm_5tsdf atm_Sers34'; // note that `cx` is dropping `atm_swer3` because it knows that the result for color should be `atm_Sers34`

// CSS Output
/*
.atm_swer3  { color: red; }
.atm_Sers34 { color: blue; }
.atm_5tsdf { font-size: 2em; }
*/

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:

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

export const className = css`
  color: red;
  & a {
    color: blue;
  }
`;

Instead, there are other options (use css from @linaria/core, or apply the styles atomically to the a)

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 add styled 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:

  • Better documentation for the new API
  • The styled API equivalent of this
  • The ability to customize the atom prefix
  • A utility in @linaria/server to extract atomic css the same way critical CSS is extracted
    • This should also take care of ordering – we should order the atoms so that @media queries appear after the other atoms
  • Build optimization cx
    • We currently "combine" atoms at runtime, but we can resolve 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 optimization

Implementation

While the API to access it exists in packages/atomic, the hard work is almost entirely done in packages/babel. The changes are to:

  1. Change isStyledOrCss to be getTemplateType, which can now be one of css | styled | atomic-css (we can add atomic-styled when we support that
  2. Do mostly the same things with atomic-css as we would with css in terms of processing the template literal and evaluating, with some exceptions:
  • Do not post process the css (conceptually, we can't support nesting with it, so the stylis default doesn't really make sense)
  • Atomize the CSS (process it with postcss, and return a list of declrations)
  • Replace the css template literal not with just a class name, but with an object that has keys (See here). This is necessary so that cx 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.

displayName: displayName!,
start: path.parent?.loc?.start ?? null,
};
path.replaceWith(atomicClassObject);
Copy link
Contributor Author

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

Copy link
Collaborator

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.

Copy link
Collaborator

@Anber Anber left a 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!

): string;
}

const cx: ICX = function cx(...rest) {
Copy link
Collaborator

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.

@@ -0,0 +1,321 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
Copy link
Collaborator

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.

@@ -0,0 +1,45 @@
import postcss from 'postcss';
Copy link
Collaborator

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.

Copy link
Contributor Author

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:

  1. 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)
  2. Invert the dependency so that @linaria/atomic has the atomize 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 in atomize: 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,
Copy link
Collaborator

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?

Copy link
Contributor Author

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);
Copy link
Collaborator

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.

@jpnelson
Copy link
Contributor Author

Thank you for your feedback @Anber ! I've updated the PR with the suggestions, primarily:

  1. cx is now shared between @linaria/atomic and @linaria/core, in @linaria/utils
  2. I moved the postcss dependency into @linaria/atomic, and added an option to the linaria config that specifies how to atomize the css text when it's given. The docs recommend setting this to atomize: require('@linaria/atomic').atomize

I'd love to hear what you think – thanks again for the feedback

@jpnelson jpnelson requested a review from Anber November 11, 2021 07:08
@jpnelson
Copy link
Contributor Author

Hi @Anber , did you have any further thoughts on this? I'd be happy to help address any more outstanding issues 🙏

@Anber
Copy link
Collaborator

Anber commented Nov 29, 2021

Hi @jpnelson!

It looks good! I'm going to merge it and release it with the next beta.

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.

None yet

2 participants