mrnocreativity / postcss-critical-split

A PostCSS plugin that takes existing CSS files and splits out the annotated critical styles into a seperate file.
MIT License
89 stars 6 forks source link

@keyframes rules appear in both or neither output files depending on inside declarations #10

Closed Jonax closed 5 years ago

Jonax commented 6 years ago

A @keyframes rule marked as critical will appear in both critical & rest CSS output if the rule in question contains declarations. Alternatively, a @keyframes rule will feature in neither if lacking declarations.

input.css

/* critical:start */
@keyframes neitherA
{
    50%
    {
    }
}
/* critical:end */

/* critical:start */
@keyframes neitherB
{
    from
    {
    }

    to
    {
    }
}
/* critical:end */

/* critical:start */
/* From https://developer.mozilla.org/en-US/docs/Web/CSS/%40keyframes */
@keyframes bothA
{
    from
    {
        margin-left: 100%;
        width: 300%;
    }

    to
    {
        margin-left: 0%;
        width: 100%;
    }
}
/* critical:end */

/* critical:start */
@keyframes bothB
{

    0%, 80%, 100%
    {
        transform:  scale(0);
    }

    40%
    {
        transform:  scale(1);
    }
}
/* critical:end */

neitherA & neitherB appear in neither critical or rest output. bothA & bothB appear in both.

Repro case was ran through gulp-postcss with no other plugins used. Modules used include:

I tried having a dig in the code in the hopes of providing a PR, but had some difficulty figuring the overall flow (e.g. appendDeclaration() vs appendFullBlock()). Closest I could find is that all affected cases seem to trigger the following condition in getAllCriticals():

  } else if (criticalActive === true && (line.type === 'decl' && hasParentAtRule(line, 'keyframes') === true)) {
    // ignore this rule...
mrnocreativity commented 6 years ago

Hey @Jonax. Thanks for reporting this. Could you provide me with your code (repo, zip, copypaste right here)? That way I can make sure that this happens due to software bugs and not due to user error. (The flow critical flow is confusing to some people).

I ran into @keyframes problems myself while using it a year ago so I specifically wrote some tests to verify the good workings of keyframes. (some for @font-face)

Considering the code: I think I might need to rewrite the code to be more readable but right now, I can't come up with better ideas (also considering processing performance).

Jonax commented 6 years ago

It took a while to do since it's part of a much bigger beast, but I've been able to isolate it into a clean repro case and throw it into a repo. Sorry if it's a little messy, but hopefully it should be self-sufficient enough to repro the bug.

https://github.com/Jonax/postcss-critical-split-issue-10

I'd like to give help on a rewrite of the keyframes, but sadly I don't know enough about the CSS model to do so (I'm a webdev neophyte, whereas my background's in gamedev). Hopefully this'll be enough instead.

Let me know if you need any more info.

mrnocreativity commented 6 years ago

Hey @Jonax ,

I just took my time to go through this error in-depth and realized that this is actually expected behaviour. I also don't really understand the use-case here.

As you have no actual declarations, this will in fact do nothing. The empty rules get deleted because critical split actually generated empty rules from time to time which get deleted afterwards during a cleanup run. I didn't expect this to be a problem.

Could you explain why you need the empty rules? That would help me understand what you're trying to do.

Thanks!

mrnocreativity commented 5 years ago

It's been more than a year since the last feedback. Considering the behavior mentioned is actually expected, I'm closing this. Let me know if this turns out to be a problem and should be revisited. :)

eni96 commented 5 years ago

Hi. I have a same issue. input.css /* critical:start */ @keyframes logo { from { transform: rotate(-45deg) scale(0); } 50% { transform: rotate(-45deg) scale(1.5); } to { transform: rotate(0deg) scale(1); } }/* critical:end */ input.critical.css @keyframes logo { from { transform: rotate(-45deg) scale(0); } 50% { transform: rotate(-45deg) scale(1.5); } to { transform: rotate(0deg) scale(1); } } input.rest.css @keyframes logo { from { transform: rotate(-45deg) scale(0); } 50% { transform: rotate(-45deg) scale(1.5); } to { transform: rotate(0deg) scale(1); } } But input.rest.css expected to be empty. Font-face and other stuff work well. The only problem is keyframes rule.