godaddy-wordpress / sake

Our internal build tool for WP/WC plugins
MIT License
4 stars 0 forks source link

Add a build task to handle Gutenberg blocks with Webpack #47

Closed wesleycole closed 5 years ago

wesleycole commented 5 years ago

Summary

Create a new blocks compilation script compile:blocks specifically for Gutenberg blocks. This is part of script tasks like watch and compile to use Webpack for bundling scripts that make use of require() etc. so libraries and third parties scripts can be used in ES6/React scripts for the blocks editor interface.

Plugins that do not add blocks to the Gutenberg editor will ignore this.

Structure

Because Webpack requires a single point of entry to create a bundle output, the script will look specifically for a single file which should be placed into the assets/js/blocks/src path of the plugin. The plugin file name can be anything, but will be used as the minified script name. For example: assets/js/blocks/src/wc-memberships-blocks.js will result in assets/js/blocks/wc-memberships-blocks.min.js which is the file PHP should register once and the blocks registered should reference to.

The file in src can include statements like require( './components/block.js' ) or external libraries for script bundling. (Note: ./components/ path is not a requirement, it can be any subdirectory)

Quirks

For some strange reason, perhaps due to Webpack living inside Sake, when compiling blocks scripts on plugins that use the compile:blocks pipe, an error may be produced. It looks like Webpack fails because it can't find some libraries that shold be in Sake instead; but Webpack wants them in the node_modules root in the plugin itself. If that's the case, the plugin should add:

    "@babel/core": "^7.5.5",
    "@babel/preset-env": "^7.5.5",
    "@babel/preset-react": "^7.0.0",
    "babel-loader": "^8.0.6",

to its package.json file's devDependencies.

Notes

@wesleycole

This creates a new compile script specifically for gutenberg blocks. The script accepts a single index.js as the entry in the assets/js/blocks directory and will output the bundled file in assets/js/blocks/dist. Webpack doesn't work well with globs and primarily needs a single entry file to compile, which is why this has a little bit different of a structure from the rest of the JS scripts.

@unfulvio

I modified Wes' script to allow compilation also on plugins that don't include yet a blocks script. Also, I changed the name convention from index.js found in assets/js/blocks to whatever script name is found in assets/js/blocks/src. The file name, in its bundled/minified form, will be used in the assets/js/blocks output. Example assets/js/blocks/src/wc-memberships-blocks.js will result in assets/js/blocks/wc-memberships-blocks.min.js. This is a departure from the index.js convention, but is closer to our WC plugins script naming conventions.

Todo

QA / Testing

unfulvio commented 5 years ago

Hey @wesleycole excellent, I was able to run a bundling on Memberships. I tweaked your PR slightly here: https://github.com/skyverge/sake/pull/47/commits/20c05445a09e287d941826c933842cbc5e8bbb1b and edited the summary in OP above

Also, this is the commit on Memberships' end that puts it together and successfully bundles a file with working blocks in it: https://github.com/skyverge/woocommerce-memberships/pull/731/commits/976e1d9b10ac9da165d7f085feae8f88744dcbc6

There's 2 remaining tasks probably: I've noticed that watch no longer listens for changes in dependencies of the bundle src file. It would be nice to trigger compile:blocks every time one changes it's reference files, but I realize this is not obvious. Not a big deal to run again compile instead.

Something necessarily to solve instead is to avoid creating a final build with build, prerelease or deploy tasks that includes js files in assets/js/blocks/src as these shouldn't be part of the release.

Finally, I noticed that in this way, with Webpack, we lose script maps. Maybe we should still compile the file unminified (not for distribution in builds) so if there's a JS error it's easier to find out which line caused it when developing.

These last two issues should be simple to solve as they concern paths. I haven't looked yet into them, but so far this looks great! Thank you

wesleycole commented 5 years ago

@jdeeburke yes, it could be applied globally but it would change up the way a lot of things are handled in Sake. If you wanted to apply this globally, I would look into replacing a lot of the Gulp pipes with parcel.js. Webpack works best when it knows what the names of the files that are going to be compiled, but that is not always the case with the plugins.

We ran into this problem with the Gutenberg blocks. Also, Webpack will always output the number of entry files that are passed in. It doesn't work well with globs. Parcel does, but would be a pretty big departure for how JS is compiled/bundled currently.

I think it would be a good move in the long run to switch to Parcel or Webpack but it might be more of an undertaking than is desired.

unfulvio commented 5 years ago

In the end for the time being I don't see require() used often, save for edge cases or mostly Gutenberg. In the future though as WooCommerce Admin enters WC core and we want to start rolling in our own React UI in other screens than Gutenberg, then probably we should look back at how we handle this.

unfulvio commented 5 years ago

Thanks for adding that, @ChaseWiseman !

I'm gonna deploy later Memberships using this branch and if everything is ok I'm gonna merge this branch in master as well.

For reference, I've found this library provided by WordPress folks to help compiling blocks code: https://www.npmjs.com/package/@wordpress/scripts -- it comes opinionated though and has assumptions about where you place your source and output - just like we do now through Sake. I don't think it's useful to us at this point, but I just wanted to share this.

jdeeburke commented 5 years ago

Tested this branch on deploying a regular plugin (Authorize.net) and it looks to have worked normally 👍

unfulvio commented 5 years ago

thanks @jdeeburke I'm gonna merge this then