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

[gh385] Minify the CSS and JS for production build #405

Merged
merged 5 commits into from
Jun 22, 2023

Conversation

sanG-github
Copy link
Contributor

@sanG-github sanG-github commented May 12, 2023

close #385

What happened 👀

We customize the build process between development and production, as we want the production build to be as minimal as possible.

Insight 📝

With JS, we config the esbuild configuration:

  • Add minify option to be true in the case we don't have the --watch option (production)

With CSS, we separate two scripts:

  • build:css for the production that included the '--style=compressed' option.
  • build:css-dev for the development environment, without the compressed option.

Proof Of Work 📹

Minify JS Minify CSS
Screenshot 2023-05-05 at 10 46 04 Screenshot 2023-05-05 at 10 52 34

Generated package.json

image

Generated Procfile.dev

image

@sanG-github sanG-github self-assigned this May 12, 2023
@sanG-github sanG-github marked this pull request as draft May 12, 2023 07:57
@sanG-github sanG-github marked this pull request as ready for review May 18, 2023 09:03
@sanG-github sanG-github force-pushed the feature/gh-385-minify-css-js branch from 2ce57f1 to aeb4bd7 Compare May 26, 2023 14:23
.template/variants/web/package.json.rb Outdated Show resolved Hide resolved
@@ -37,9 +37,16 @@
'--no-source-map',
'--load-path=node_modules'
]
production_bundled_stylesheet_options = bundled_stylesheet_options + [
'--style=compressed'
]

run %(npm set-script build "node app/javascript/build.js")
run %(
npm set-script build:css \
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be best to use a prefix here and avoid it below:

npm set-script build:css-production \

So production is special, while dev is the base/standard mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! The default one should be the development environment.

I've updated it in 2c9789c

Copy link
Member

Choose a reason for hiding this comment

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

The build:css-production won't be trigger as no one call that, the https://github.com/rails/cssbundling-rails/blob/main/lib/tasks/cssbundling/build.rake#L20 enhance the css:build by default, we need to go with the Sang's original solution FYI @olivierobert I will open a PR for that cc @sanG-github

image

@sanG-github sanG-github requested a review from olivierobert May 30, 2023 11:47
sanG-github added a commit that referenced this pull request May 30, 2023
Copy link
Member

@malparty malparty left a comment

Choose a reason for hiding this comment

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

🔥 🚀 🔥

@@ -6,6 +6,7 @@ require('esbuild')
inject: ['app/javascript/global.js'],
bundle: true,
sourcemap: true,
Copy link
Member

Choose a reason for hiding this comment

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

@sanG-github a small comment, we really don't use source map on Production, should we set this as

Suggested change
sourcemap: true,
sourcemap: !watch,

Copy link
Contributor

Choose a reason for hiding this comment

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

Wd do use sourcemap 😅 You might not but sourcemaps as essential for debugging

Copy link
Contributor

Choose a reason for hiding this comment

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

@olivierobert , that's interesting, we have --no-source-map for sass, for all environments

Then, I tried enabling it for a local project, and it was trying to reference the file by its path 😄

image

I searched around and it seems like an existing issue for cssbundling-rails

I was wondering if you ever were able to use source maps for SCSS assets in a Rails project?

Copy link
Member

Choose a reason for hiding this comment

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

@tyrro Nice catch! :o Should we create a dedicated issue to investigate this?

Copy link
Member

Choose a reason for hiding this comment

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

@olivierobert but we don't debugging on Production 🤔

Copy link
Member

@andyduong1920 andyduong1920 Jun 16, 2023

Choose a reason for hiding this comment

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

ok, after checking this article https://css-tricks.com/should-i-use-source-maps-in-production/ it's better to have the source map information in the Production build cc @tyrro @olivierobert @malparty

There are 2 benefits

image

Copy link
Member

Choose a reason for hiding this comment

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

@malparty if we have a chance, we could just remove this config on the same PR ;)

image

Copy link
Contributor

Choose a reason for hiding this comment

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

@andyduong1920 , but source-map doesn't work for cssbundling-rails (please refer to my last comment).

So if we remove --no-source-map option, it'll try to generate the source map, which will result in the following:

Screen.Recording.2023-06-16.at.4.30.16.PM.mov

which is not very helpful for debugging 😄

@malparty , IMHO it's not worth it at this moment.
jsbundling-rails support source-map (and we are using it too btw), css-bunding-rails yet to support it. So, I'd keep it as it is regarding the --no-source-map option!

Copy link
Member

Choose a reason for hiding this comment

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

@andyduong1920 lets create an issue for that :)

Copy link
Member

Choose a reason for hiding this comment

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

Issued created #414 @malparty @tyrro

@sanG-github sanG-github force-pushed the feature/gh-385-minify-css-js branch from 2c9789c to 9639680 Compare June 16, 2023 07:43
@malparty
Copy link
Member

@sanG-github rebase needed 🙇

@malparty
Copy link
Member

@tyrro rebase needed and then we can merge this one 🚀

@tyrro
Copy link
Contributor

tyrro commented Jun 21, 2023

@malparty , I'll tag @sanG-github so that he can do the rebasing for me 🙈 😏

@sanG-github sanG-github force-pushed the feature/gh-385-minify-css-js branch from 9639680 to 4d587c1 Compare June 21, 2023 09:17
@malparty malparty merged commit 7c837f9 into develop Jun 22, 2023
@malparty malparty deleted the feature/gh-385-minify-css-js branch June 22, 2023 03:09
@malparty malparty mentioned this pull request Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minify the CSS and JS for Production build
5 participants