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

Refactor keyframe purge to use the postcss api #34

Merged
merged 8 commits into from Jan 12, 2018

Conversation

jsnanigans
Copy link
Member

@jsnanigans jsnanigans commented Dec 19, 2017

Proposed changes

Use postcss to remove unused keyframes

Types of changes

  • Refactor
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@Ffloriel
Copy link
Member

Ffloriel commented Dec 20, 2017

Thanks, this is great. There are some improvement that can be made related to performance.

When the keyframes option is set to true, the css is parsed 2 times during the purge process. Parsing the file can be costly in term of performance. The idea is to parse the file is the beginning and work with the AST (root). At the end of the process, we create the css based on the AST (root.toString()).

One other thing to get into consideration is the number of time purgecss will walk through the AST. Each walk.. method is calling the walk method. So we are walking 2 times to remove the keyframes and 1 time to remove the unused selectors.

I don't think we can avoid walking through the AST less than 2 times because we need to gather the used keyframes first. But we can combine removing the unused css and unused keyframes.

@jsnanigans
Copy link
Member Author

Good point, I defined a root variable in the class object, this will be changed when there are multiple CSS but I think that's ok, it will let the garbage collector remove the old one, or else we could just define the root in getCssContents and then pass it to the purging functions (just revert to the commit
932422a).

I'm not sure how we can combine the walks, since we need to run walkDecls after the unused selectors are purged and before the walkAtRules is run that removes the keyframes, also I think that walkAtRules is not as expensive as walkRules since it only applies to the ones that match the regex, but I have not tested that.

@Ffloriel
Copy link
Member

Passing the root to the purging functions is the choice I would make but it is just a detail at this point.
Ideally, the purgecss api could be more functional. Allowing developers to pick useful functions from it and import them in their code.

import { getSelectorsInCss } from 'purgecss'

But that is not a priority and defining a root variable in the class object is ok.

To combine the walks,, you need to look at the postcss api code. Here is the walkDecls

if ( !callback ) {
            callback = prop;
            return this.walk( (child, i) => {
                if ( child.type === 'decl' ) {
                    return callback(child, i);
                }
            });
        }

You can see that the function calls this.walk with a callback that check the type of chid. Looking at all the other walk.. method, you will see different type but the principle is the same. So the simple way to combine the walks is to use root.walk directly.

Ffloriel
Ffloriel previously approved these changes Jan 11, 2018
@Ffloriel Ffloriel merged commit 37510de into master Jan 12, 2018
@Ffloriel
Copy link
Member

Thanks again for the PR. I will release a new version on npm with the changes this week-end.
After that, we can make the modification on the documentation.

@jsnanigans jsnanigans deleted the feature/keyframes_refactor branch January 12, 2018 08:07
Ffloriel added a commit that referenced this pull request Oct 30, 2019
Update rollup to the latest version 🚀
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