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

WordPress Scripts Upgrade #728

Merged
merged 23 commits into from
Sep 16, 2021
Merged

Conversation

gregrickaby
Copy link
Contributor

@gregrickaby gregrickaby commented Aug 23, 2021

Closes #731

DESCRIPTION

@wordpress/scripts now supports Webpack 5, which means we can finally upgrade some very outdated packages and use PostCSS 8!

Here are the Node dependencies that were upgraded:

 @wordpress/scripts                                            ^17.1.0  →  ^18.0.0     
 copy-webpack-plugin                                             6.4.1  →    9.0.1     
 css-loader                                                     ^5.2.6  →   ^6.2.0     
 css-minimizer-webpack-plugin                                    1.3.0  →    3.0.2     
 eslint-webpack-plugin                                          ^2.5.4  →   ^3.0.1     
 mini-css-extract-plugin                                        ^1.6.2  →   ^2.2.0     
 postcss-loader                                                  4.2.0  →    6.1.1     
 sass-loader                                                    10.2.0  →   12.1.0     
 stylelint-webpack-plugin                                       ^2.2.2  →   ^3.0.1     
 svg-spritemap-webpack-plugin                                    3.9.1  →    4.2.0     
 tailwindcss                                                     2.2.4  →   ^2.2.7

Other

This was appearing in the terminal and is coming from the svg-spritemap plugin. It's bug #175. The plugin author has committed a fix directly to master, but has yet to release it under semver.

"extendDefaultPlugins" utility is deprecated.
Use "preset-default" plugin with overrides instead.
For example:
{
  name: 'preset-default',
  params: {
    overrides: {
      // customize plugin options
      convertShapeToPath: {
        convertArcs: true
      },
      // disable plugins
      convertPathData: false
    }
  }
}

I've slapped a band-aid on this by pointing our package.json file directly at the commit hash with the fix. This is fugly, but works, and should be considered a temporary until the fix is actually released and properly semver'd.

I've also updated composer dependencies and adjusted the webpack.config files to use Webpack 5 shape and syntax.

SCREENSHOTS

Front-end:
screenshot

Build:
screenshot

STEPS TO VERIFY

How do we test this?

  1. gh pr checkout 728
  2. npm ci
  3. npm run build
  4. Verify everything compiles and appears on the front-end correctly
  5. npm run dev
  6. Verify everything in "dev mode" works correctly
  7. npm run lint
  8. Verify linting still works as expected
  9. npm run format
  10. Verify formatting still works as expectedd

@gregrickaby gregrickaby marked this pull request as ready for review August 23, 2021 14:39
@coreymcollins
Copy link
Contributor

Everything seems to be good here, except when I run npm run format I get this:

npm ERR! cb.apply is not a function
.
npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/coreycollins/.npm/_logs/2021-08-27T14_58_11_104Z-debug.log
..Install for [ 'markdownlint-cli@latest' ] failed with code 1
ERROR: "format:md" exited with 1.

The debug log offers this info:

9 silly pacote tag manifest for markdownlint-cli@latest fetched in 587ms
10 verbose stack TypeError: cb.apply is not a function
10 verbose stack     at /Users/coreycollins/node_modules/npx/node_modules/npm/node_modules/graceful-fs/polyfills.js:287:18
10 verbose stack     at FSReqCallback.oncomplete (fs.js:184:5)
11 verbose cwd /Users/coreycollins/Local/wdunderscores/app/public/wp-content/themes/wd_s
12 verbose Darwin 20.6.0
13 verbose argv "/usr/local/bin/node" "/Users/coreycollins/node_modules/npx/node_modules/npm/bin/npm-cli.js" "install" "markdownlint-cli@latest" "--global" "--prefix" "/Users/coreycollins/.npm/_npx/9308" "--loglevel" "error" "--json"
14 verbose node v14.16.1
15 verbose npm  v5.1.0
16 error cb.apply is not a function
17 verbose exit [ 1, true ]

Is this anything you've run into before?

@gregrickaby
Copy link
Contributor Author

@coreymcollins Ah, looks like you're using NPM 5. Please upgrade to NPM 7 (npm i -g npm@latest) and try again.

@coreymcollins
Copy link
Contributor

Weird, my version of npm locally and globally are at 7.10.0:
image

I'll have to do some digging over here and see what's going on with that.

@coreymcollins
Copy link
Contributor

Was finally able to resolve my weird npm incorrect version issue, so this is all working for me at this point sans the merge conflicts.

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.

Doing some final testing before a manual merge of this to resolve the conflicts, I noticed two things that I hadn't before.

  1. CSS sourcemaps don't seem to be working anymore when running watch/dev or start. Instead, everything points to index.css instead of the individual partials.
  2. A few SVG images are getting dropped into the top-level /build/ directory when I build. Screenshot below of what I'm seeing in my build folder. This happens when running any build/watch/dev/start.

image

@coreymcollins
Copy link
Contributor

Digging into the SVG image thing some more, I've found this:

The 3 SVGs that are being dumped into the top-level build directory are hamburger.svg, caret-down.svg, and close.svg. The commonality between these is that all 3 are used as background images in Sass partials. If I remove the styles where one of them is set as a background image, the SVG for that image no longer appears in the build directory when running our build scripts.

This is weird! Also something that isn't happening on the current main branch, but there's nothing obvious here that jumps out at me to explain why this is happening.

@coreymcollins
Copy link
Contributor

coreymcollins commented Sep 9, 2021

I think I've been able to track the SVG problem down to css-loader. The weird thing it's doing with the SVGs happens when we're using 6.2.0, which is where it's set on this branch.

If I revert back to the current wd_s version of ^5.2.6, the SVG weirdness doesn't happen and everything else seems to continue working as expected.

Going to look into what changed in the versions since 5.2.6 and if there's some option to set to keep this from happening.

Edit: this isn't specific to SVGs, just as a heads up. We just only happen to be using SVGs as backgrounds in our partials, but if you use a PNG or JPG it will also do the same thing of creating a version of that image in the build directory.

@coreymcollins
Copy link
Contributor

So it looks like url-loader and file-loader were both deprecated, which we were using both of.

I've updated per the instructions here: https://github.com/webpack-contrib/css-loader/blob/master/CHANGELOG.md (which points to https://webpack.js.org/guides/asset-modules/)

Running into a hiccup with fonts, but images are working as expected now.

@coreymcollins
Copy link
Contributor

Got it! That latest commit should resolve the duplicate SVG issue and replace the loader used for fonts.

@coreymcollins
Copy link
Contributor

At this point, the remaining issue is with sourcemaps. I'll try and dig into that today as well.

@coreymcollins
Copy link
Contributor

Okay!

I think the above two commits solve the sourcemap issue. I had to bump mini-css-extract-plugin from 2.2.0 to 2.2.2. I then also simplified the various loaders in the webpack.config.js file and everything seems to work as expected.

Could use some testing to be sure, but this seems to be working on my end.

# Conflicts:
#	package-lock.json
#	package.json
@gregrickaby
Copy link
Contributor Author

@coreymcollins I was final able to resolve the merge conflict!

@coreymcollins
Copy link
Contributor

Excellent! If everything else works as expected for you here, I can give this the ole 👍🏻 and merge this sucker in.

@gregrickaby
Copy link
Contributor Author

gregrickaby commented Sep 16, 2021

@coreymcollins My testing this morning was weird. If I made a change to a Sass partial one-folder deep, Webpack would compile everything fine...but, if I made a change to a Sass partial two-folders deep, it wouldn't see the change and nothing recompiled.

I found this PR WordPress/gutenberg#34264 which addresses the issue. So I bumped @wordpress/scripts to 18.0.1, re-tested, and everything compiled and recompiled as expected.

@coreymcollins
Copy link
Contributor

Ooh, nice catch! If everything else is looking good from your testing, I'll go ahead and merge this in this morning.

Thanks!

@coreymcollins coreymcollins merged commit 155b8f3 into main Sep 16, 2021
@coreymcollins coreymcollins deleted the feature/wordpress-scripts-upgrade branch September 16, 2021 13:31
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.

2 participants