Skip to content
This repository has been archived by the owner on May 15, 2024. It is now read-only.

feat(styles-loader): disable purgecss by default #614

Merged
merged 8 commits into from
Mar 11, 2024

Conversation

zacowan
Copy link
Member

@zacowan zacowan commented Feb 27, 2024

Description

Makes purgecss disabled by default, requiring users to explicitly opt-in to purgecss by setting bundler.purgecss.enabled to true, or bundler.purgecss.disabled to false. Additionally, removes the check for NODE_ENV === 'production' when executing purgecss to ensure a consistent experience between different environments.

Motivation

purgecss has been a common point of confusion for users of one-app. During development, users typically bundle using the dev bundler, which does not purge any css. After development, users will switch to the production bundler, at which point css is purged as an optimization technique.

This works great, but only if perfect defaults are supplied to the purgecss plugin for which content to analyze for unused selectors. purgecss starts by assuming that 0 selectors are used, and then adds any selectors it finds being used, which typically results in a few cases where selectors are removed that are actually being used. Because modules can be so diverse, it's not possible to provide perfect defaults without incurring massive performance penalties on bundle times.

As a result, users will often notice (later than they might like) that css gets purged from their module that they don't expect to be purged. Because this is an optimization technique, it is reasonable to let users choose to opt-in to purgecss, with the understanding that they must customize the config to ensure that only unused css classes get purged from their bundle.

Test Conditions

Modified existing unit tests to check that css is purged only when explicitly enabled. Additionally, added new tests to verify that purgecss executes when NODE_ENV is set to development.

Types of changes

  • Bug fix (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)
  • Documentation (adding or updating documentation)
  • Dependency update
  • Security update

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • These changes should be applied to a maintenance branch.
  • I have added the Apache 2.0 license header to any new files created.

@zacowan zacowan marked this pull request as ready for review March 6, 2024 15:14
@zacowan zacowan requested review from a team as code owners March 6, 2024 15:14
code-forger
code-forger previously approved these changes Mar 6, 2024
@code-forger code-forger requested a review from a team March 7, 2024 07:20
Matthew-Mallimo
Matthew-Mallimo previously approved these changes Mar 7, 2024
@10xLaCroixDrinker
Copy link
Member

@code-forger @Matthew-Mallimo so we are planning on a major version bump for this?

@code-forger
Copy link
Member

@10xLaCroixDrinker I was hoping to consider this a bug in 7.0.0 of the bundler. And rely on the fact that 7.0.0 is barely consumed by anyone yet. (which we know because most real modules cant use 7.0.0 until #616 is released)

It would be very unfortunate to have to version bump immediately after a major.

@zacowan zacowan dismissed stale reviews from Matthew-Mallimo and code-forger via e09e75c March 7, 2024 16:12
code-forger
code-forger previously approved these changes Mar 7, 2024
@zacowan zacowan requested a review from JAdshead March 8, 2024 17:05
Copy link
Member

@giulianok giulianok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm interested in purgecss starts by assuming that 0 selectors are used, and then adds any selectors it finds being used, where did you find that? documentation? code?

@giulianok
Copy link
Member

I'm interested in purgecss starts by assuming that 0 selectors are used, and then adds any selectors it finds being used, where did you find that? documentation? code?

I think that's confusing the the work used. Just to clarify, PurgeCSS does the following:

  1. Reads all the css files and extracts the selectors
  2. Traverses the AST checking every single selector, and removes styles when can't find a match for a particular selector

@10xLaCroixDrinker
Copy link
Member

@10xLaCroixDrinker I was hoping to consider this a bug in 7.0.0 of the bundler. And rely on the fact that 7.0.0 is barely consumed by anyone yet. (which we know because most real modules cant use 7.0.0 until #616 is released)

It would be very unfortunate to have to version bump immediately after a major.

I'm not stoked about it, but I'm okay with this. However, the commit type MUST be changed to fix.

@code-forger code-forger merged commit 0955cf3 into main Mar 11, 2024
5 checks passed
@code-forger code-forger deleted the feat/make-purgecss-opt-in branch March 11, 2024 18:23
@zacowan
Copy link
Member Author

zacowan commented Mar 11, 2024

I'm interested in purgecss starts by assuming that 0 selectors are used, and then adds any selectors it finds being used, where did you find that? documentation? code?

I'm interested in purgecss starts by assuming that 0 selectors are used, and then adds any selectors it finds being used, where did you find that? documentation? code?

I think that's confusing the the work used. Just to clarify, PurgeCSS does the following:

  1. Reads all the css files and extracts the selectors
  2. Traverses the AST checking every single selector, and removes styles when can't find a match for a particular selector

I had heard about this and sort of just took it at face value 😅 agreed that my original statement isn't true, thanks for flagging and clarifying 😄

A better way to describe the thinking behind that statement is that we must provide a complete list of ASTs (or paths to files) to purgecss that could reference a css class. If an AST is not provided to purgecss that references a class, then any of the classes used by that AST will be considered unused and purged (unless referenced in some other file).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants