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

Migrate off purify-css #10536

Closed
benmccann opened this issue Aug 11, 2023 · 5 comments · Fixed by #10546
Closed

Migrate off purify-css #10536

benmccann opened this issue Aug 11, 2023 · 5 comments · Fixed by #10546
Labels
help wanted PRs welcomed. The implementation details are unlikely to cause debate

Comments

@benmccann
Copy link
Member

Describe the bug

purify-css has not been updated in 6 years and is causing security warnings in the AMP example

Reproduction

css = purify(markup, css);

Logs

No response

System Info

`master`

Severity

annoyance

Additional Information

Some alternatives:

@benmccann benmccann added the help wanted PRs welcomed. The implementation details are unlikely to cause debate label Aug 11, 2023
@ghostdevv
Copy link
Member

going to look into it now 🙏

@ghostdevv
Copy link
Member

So looking into it, I'm not sure why it's included in the tests when it's not included in the docs. #4710 has a comment about why you might need it:

One thing we lose is the ability to exclude CSS from components that are imported but not rendered for a given page. Given AMP's wholly arbitrary CSS size limit, that can be a problem. This can be solved (at least for prerendered pages, where it doesn't matter how expensive this sort of processing is) with something like purifycss/purifycss:

What is the preferred solution here?

  • Migrate off purify css
  • Migrate off purify css and add a note about this to the docs
  • Remove it altogether

Will raise a PR for whatever is decided - tagging @Rich-Harris here too as he originally added this 🙏

@ghostdevv
Copy link
Member

ghostdevv commented Aug 12, 2023

Some alternatives:

leeoniya/dropcss:

  • No types
  • CJS build only

FullHuman/purgecss:

  • Uses Node Libs
  • Big dependancies

@benmccann
Copy link
Member Author

I think using dropcss might be the best solution then as I'd prefer not to pull in a lot of dependencies and it'd be nice to demonstrate usage for our users. I filed an issue there requesting some types, but in the meantime we can just do // @ts-ignore if needed

If that doesn't work for some reason then my second choice would be to remove it and add a comment like:

// you could invoke a library like purify-css/purgecss/dropcss here to exclude CSS from components that are imported, but not rendered

@ghostdevv
Copy link
Member

I filed an issue there requesting some types, but in the meantime we can just do // @ts-ignore if needed

Ty! I ended up writing types for the tests in #10546 so submitted a pr to dropcss to add those types in - hopefully they take a look at it soon 🤞

dummdidumm added a commit that referenced this issue Aug 23, 2023
Closes #10536

Switch to using dropcss
Add note about removing css in the docs
Add note in docs about adding <html amp> as we don't do that anywhere. I also want to discourage people from using ⚡ since dropcss doesn't support it - people definitely will be using it so I've added a safeguard (.replace('⚡', 'amp'))

---------

Co-authored-by: Puru Vijay <[email protected]>
Co-authored-by: Simon H <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted PRs welcomed. The implementation details are unlikely to cause debate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants