fix(atomic): slugify keyframes and enable vendor prefix

[hackwaly]

Motivation

Close #990

Summary

Test plan

[Anber]

@jpnelson @juanferreras, may I ask you to take a look at this PR?

[juanferreras]

@Anber sure! sorry on the delay.

There are 2 changes here that I think it'd be wise to split:

1. Forcing vendor prefixing – I don't personally think in 2022 this is a good default (especially as users wouldn't be able to opt out). I know Linaria has it enabled in general, but it's still possible to disable by doing things like Allow to disable auto prefixng for extracted CSS #57 (comment) (which might be quite common in userland).

Between not supporting vendor prefixing in atomic, and forcing it without being able to disable – I think we might still prefer the latter or a more in-depth discussion (eg. if we should make it default to no vendor prefix and configurable to opt back in)

2. Atomic Animation local namespacing – I'd defer to @jpnelson or @Anber on this. Do note that it is currently documented in docs/ATOMIC_CSS.md and also in the code of atomize.ts

The main benefit of the current way is that you can define the @keyframes once (anywhere) and then provided you're using it by name works:

import { css } from '@linaria/atomic';
const x = css`
  @keyframes fade {
    from { opacity: 0; }
    to { opacity: 1; }
  }
  animation: fade 1s infinite;
`;
// note this could be on the same file or not
const y = css`
  animation: fade 1s infinite;
`;

I understand @hackwaly your current implementation would not support that 2nd animation declaration (it'd mangle it to a non-existing animation-name), and would instead always require repeating the @keyframes definition, but LMK if I'm mistaken.

I think unless we could support using the right slug everytime, it might be better to continue relying on the @keyframe <name> from being purposeful and unique and improving the documentation as to why.

[hackwaly]

This PR is just trying to make @linaria/atomic consistent with @linaria/core.

After I switched my project from @linaria/core to @linaria/atomic. I found that two things broken. One is vendor prefix, the other is keyframe names.

Keyframe names defined in difference css`` with same name are collided. I must manage keyframe names with care. Otherwise only one keyframe of collided keyframes works.

Maybe we can use some name convention to control whether a keyframe name should be mangled. Eg. keyframe _mangleMe {}.

About vendor prefix. I completely agree with what @juanferreras said.

[sachin-hg]

The main benefit of the current way is that you can define the @keyframes once (anywhere) and then provided you're using it by name works:

@juanferreras we ran into similar issues. But regarding the statement you quoted above, our team had a thought let say for the given example:

import { css } from '@linaria/atomic';
const x = css`
  @keyframes fade {
    from { opacity: 0; }
    to { opacity: 1; }
  }
  animation: fade 1s infinite;
`;
// note this could be on the same file or not
const y = css`
  animation: fade 1s infinite;
`;

if someday the const x becomes unused, and we have our build pipelines setup in a way that we remove all the unused variables before linaria gets to process the file, then the animation fade won't work any longer, simply because the keyframe associated with it was dropped.

this issue wont just be limited to csaes when such things are integrated in the build pipeline. in a team of 30-40 developers, anyone could easily end up removing an unsed JS declaration. And hence they might accidentally cause production issues

Therefore, I guess the implementation in this PR is somewhat critical. I completely agree with the comment on prefixer though, but I guess we should be taking these somewhere as a parameter in linaria config

Please do let me know if I am missing a bigger picture here

[jpnelson]

Sorry for the delay here too –

1. Vendor prefixing

I don't personally think in 2022 this is a good default (especially as users wouldn't be able to opt out). I know Linaria has it enabled in general, but it's still possible to disable by doing things like #57 (comment) (which might be quite common in userland).

I agree with this thought too, I think that's the direction we should be going too.

2. Keyframe namespacing

I agree, I think the ideal solution would be that animation keyframe names are slugified, based on the hash of their contents (so, duplicates could possibly be removed if there are exact duplicates).

So for this PR, I'd take out the vendor prefixing, and ideally also add some tests so we're on the same page about what the behavior is for keyframes.

Thanks for adding this @hackwaly !

[hackwaly]

I'm sorry that I've back to linaria/core from linaric/atomic. The mainly reason is cx for linaria/atomic is too expensive. I have no motivation on this PR ever.

[jpnelson]

Good to know, thank you! For the expense of it, do you mean the expense of merging the atoms in cx? There's some optimization we could do there for basic cases, like when classes are specified in the same file as they're used (or even imported possibly), we could pre compute the result. We'd still need the merging logic in cx for the dynamic cases though. I'll make an issue here for that too, but I'm curious to know if it would make a difference for your use case.

…

On Thu, Jul 21, 2022, 8:10 AM 文宇祥 ***@***.***> wrote: I'm sorry that I've back to linaria/core from linaric/atomic. The mainly reason is cx for linaria/atomic is too expensive. I have no motivation on this PR ever. — Reply to this email directly, view it on GitHub <#992 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALSYDHU2LUX3STJYW7Q5ELVVFR7ZANCNFSM5Y7LQIZA> . You are receiving this because you were mentioned.Message ID: ***@***.***>