woocommerce / storefront

Official theme for WooCommerce
https://wordpress.org/themes/storefront/
959 stars 472 forks source link

JS files are not being minified on build #2110

Closed vijuarez closed 11 months ago

vijuarez commented 11 months ago

Describe the bug

I started working on a personal fork of storefront (just modifications specific to my use case) and got 404 errors when trying to fetch minified versions of the js files. I don't have any "optimization' plugins or anything, so I figured that the minified files should come bundled with the theme. I checked, and the .min.js files aren't being included in my build when running npm run build, even when the build scripts implies that the minifying is happening.

I digged a little more, and noticed that the minified files directories are stored in the $npm_package_assets_js_src environment variable, which is seemingly not defined anywhere, and according to Google only exists in this project. To be more precise, the build:js command executes the following:

echo \"$(tput setaf \"3\")Building JS Files$(tput sgr0)\"; for f in $npm_package_assets_js_src; do file=${f%.js}; echo \"$(tput setaf \"3\")Building $f$(tput sgr0)\"; node_modules/.bin/uglifyjs $f -c -m > $file.min.js; done

But in my environment, the $npm_package_assets_js_src variable is empty, which results in no minified files. I imagine that this variable is properly set in the devs environment, which made this bug slip through the cracks (or I'm missing something obvious)

I ended up making a fix quick with some find magic:

"prebuild:js": "find assets/js -name '*.min.js' -delete", "build:js": "echo \"$(tput setaf \"3\")Building JS Files$(tput sgr0)\"; find assets/js -name '*.js' -exec echo \"$(tput setaf \"3\")Building {}$(tput sgr0)\" \\; -exec sh -c 'node_modules/.bin/uglifyjs $1 -c -m -o ${1%.*}.min.js' sh {} \\; ",

But I'd love to know if this is a problem, or if I'm doing something terribly wrong in my environment. Thanks!

PD: There's an equally magical variable for CSS, $npm_package_assets_css_min, but that one doesn't seem to break the build for me. It might be skipping the autoprefixer and rtlcss steps of the build because of this, but I honestly haven't checked.

Isolating the problem (mark completed items with an [x]):

To Reproduce

Steps to reproduce the behavior:

  1. Run npm run build
  2. Check if the .min.js files are inside the zip

Expected behavior

The minified files should be included

Browser Environment

Please provide as much detail as possible about your testing environment.

WordPress Environment

Irrelevant

nefeline commented 11 months ago

Hi there @vijuarez , thanks for reaching out to us!

I'm not able to reproduce this issue over here, when running npm run build the minified js files are added as expected:

Screenshot 2023-07-23 at 09 45 02

Did you run both composer install and npm install before triggering npm run build ?

vijuarez commented 11 months ago

Hi @nefeline, thank you for looking into this!

I double checked my environment, and both packages from composer and npm are installed. Some ideas:

  1. Can you try deleting the minified files, and then building? They should reappear on build, if I understand the script correctly.
  2. Could you figure out where npm_package_assets_js_src is defined?

Thanks.

roykho commented 11 months ago

Hi @vijuarez - before running npm run build, try running nvm use, npm install and then npm run build. In case you don't have nvm. Here is more information on that https://github.com/nvm-sh/nvm

vijuarez commented 11 months ago

Hi @roykho, you are right! Switching to Node v14.21.1 solved the issue.

clb92 commented 11 months ago

I'm still having this issue, even with Node v14.21.1. I use NVM (nvm use in the storefront directory) when building, and it does indeed spot the .nvmrc and use the correct version of Node, yet minified files are not generated for me. I have tried with a handful of different NPM versions as well.

Between each build attempt I clear my working directory completely, clean NPM cache, and clone the theme repository again.

The NodeJS ecosystem is a mess.

And the only reason I need to build my own theme in the first place is that I want to change the breakpoints (see #991).