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

Convert css nano to conditional include #601

Merged
merged 2 commits into from
Mar 19, 2021

Conversation

michealengland
Copy link
Contributor

@michealengland michealengland commented Mar 16, 2021

Does not have open issue, must be tested locally.

DESCRIPTION

This feature improves performance when using npm run start. By preventing css-nano from running in development, rebuild times are quicker due to less processing needed.

OTHER

  • [NA] Is this issue accessible? (Section 508/WCAG 2.0AA)
  • Does this issue pass all the linting? (PHPCS, ESLint, SassLint)
  • [NA] Does this pass CBT?

STEPS TO VERIFY

  1. Before checking out this branch take an inventory of build times when using npm run start on main. Set steps below for details on rebuild.
  2. Next, checkout this feature/improve-build-tool-speed and run npm run start.
  3. Once the script has built, edit an @apply css statement. Simply commenting out files may give false positives as these changes are incredibly fast regardless.
  4. Verify that your build/style-index is unminified when using npm run start.
  5. Verify that your build/style-index is minified when using npm run build.

DOCUMENTATION

No additional documentation is required.

@michealengland michealengland marked this pull request as ready for review March 16, 2021 21:42
@bradp
Copy link
Contributor

bradp commented Mar 17, 2021

This could be simplified to be:

module.exports = {
	plugins: [
		require("tailwindcss"),
		require("autoprefixer"),
		process.env.NODE_ENV === "production" ? cssnano({ preset: "default" }) : null,
	],
};

@michealengland
Copy link
Contributor Author

This could be simplified to be:

module.exports = {
	plugins: [
		require("tailwindcss"),
		require("autoprefixer"),
		process.env.NODE_ENV === "production" ? cssnano({ preset: "default" }) : null,
	],
};

@bradp I like the simplification, but I also did some research on this and think we maybe should deviate slightly from what you have shown here.

Using process.env.NODE_ENV === "production" ? cssnano({ preset: "default" }) : null results in a null value left behind and using process.env.NODE_ENV && "production" ? cssnano({ preset: "default" }) results in a falsey value left behind. From what I'm seeing neither of these values should be passed into the Webpack plugins, see webpack/webpack#5493 (comment).

As an alternative, maybe we should follow the example seen here and fully remove false values from the plugins webpack/webpack#5493 (comment).

What are your thoughts? On a side note, I did a test and passing null did work as expected but it might be good to error on the side of caution.

@bradp
Copy link
Contributor

bradp commented Mar 18, 2021

I'd say 👍 to running it through the .filter(Boolean)

My main goal is to keep it more readable and in one place, so that works for me.

Since the ternary operator leaves behind a false value, we should filter
this out since it is unwanted.

This update is based on webpack/webpack#5493
Copy link
Contributor

@coreymcollins coreymcollins left a comment

Choose a reason for hiding this comment

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

Nice, this does bring about a nice little boost! On main, npm run start took about 20 seconds and in this branch it's around 15. It also dropped the compile time while running npm run start by about 1-1.5 seconds, fluctuating between the two.

Thanks!

@coreymcollins coreymcollins merged commit fcc82ca into main Mar 19, 2021
@coreymcollins coreymcollins deleted the feature/limit-css-nano-to-production-mode branch March 19, 2021 16:03
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.

3 participants