Yoast / wordpress-develop-mirror

WordPress development sync to develop patches against.
https://make.wordpress.org/core/
5 stars 3 forks source link

Reorganize Core JS / introduce build step (with clean git moves) #97

Closed omarreiss closed 6 years ago

omarreiss commented 6 years ago

Ticket open here: https://core.trac.wordpress.org/ticket/43055

In light of figuring out what a decent process for bundling would be, I've felt the need to zoom out more and look at the way we could organize our JS code in general. I've spent quite some hours thinking about and discussing the way the JS in WordPress core could be organized. I just wanted to expose higher level concerns that need to be addressed. I'm opening this PR to give the gist of it, but take a look at the branch I've created to see what this would amount to technically.

For this document I've consulted @youknowriad, @aduth and @adamsilverstein in #core-js and on WCUS and @atimmer and @herregroen at Yoast HQ.

A build step for WordPress core developent, what would change?

This patch will introduce a build step to WordPress core development. Once this gets committed, it will no longer be possible to run WordPress from the src directory. To make this transition easier for developers, we've added a special page that people running WordPress from source will start seeing:

screen shot 2018-01-09 at 01 00 17

As the screen says, npm install && grunt build will be enough to trigger a build.

The concerns leading to this change

1. Need to maintain backwards compatibility while providing flexibility.

We've discussed we want all enqueued scripts to still be built and present in the respective wp-admin/js and wp-includes/js. As they are there right now, they are really hard to manage. The way the code is organized is very hard to understand and it is very hard to refactor anything. To break out of the current "mess", it would be nice to have all JS source in one directory, from which it is copied or bundled to its legacy location. It's fully backwards compatible but adds an important first layer of flexibility that'll help advance JS development in core.

2. Need to separate scripts that end up in WordPress from source code and provide freedom in working with the source and organizing it in the best way.

Now that we have all the JS together in one directory, it will still be a mess to change things or split things up. It would not be transparent what would end up being enqueued and what is the source for those enqueues. In an ideal world, enqueued scripts are nothing more than a sort of manifests that load source code, initialize objects and perhaps set a few hooks. They would ideally not contain actual behavior. As we currently don't have a way to distinguish between enqueues and source code, basically everything right now is an enqueue. With the media library we've tried to demonstrate how this could work when you separate the source away. The media library then doesn't assign itself to the wp global (see src/js/media/models.js example). Instead this is then done in the enqueue (src/js/_enqueues/wp/media/models.js example). In fact It's practically all the enqueued script does.

3. Need to organize the enqueued scripts in a better way.

In the current setup, vendor scripts are mixed with self contained utility or lib scripts, deprecated scripts that are no longer in use and "other" scripts that are so all over the place that they are hard to classify. It's very hard to understand what's going on from looking at the directory structure. A large part of scripts simply assign something to the wp namespace. Why not have a directory structure for scripts which just follows the namespace? It also immediately becomes clear what should be the responsibility of those files, namely to assign modules to the namespace. The modules themselves could be moved to the src/js root and split up easily if deemed useful. This way it will be easy to explain the upgrade path towards a more modern code base. Things can simply be extracted / abstracted in multiple iterations.

Not every script assigns something to the wp namespace. We've come up with a directory structure that seems useful to us, but anything is possible really. A very nice advantage is that deprecated and vendor scripts become much easier identifiable. This what it could look like:

screen shot 2017-12-19 at 14 42 59

4. Need to manage vendor scripts better

In the current setup. Vendor scripts that exist as NPM packages are actually shipped with WordPress and managed with a sort of copy/paste system. Managing these with NPM and having a copy task or Webpack move them into the right place makes so much more sense. Most of the NPM packages simply ship with the dist files, so we only have to copy them to the right location. But now we actually manage those with NPM and they are listed in the package.json, which to me seems like a huge improvement.

Sometimes vendor scripts need to be built after install. This holds true for maybe 2 or three dependencies. This can simply be part of the build task.

Some vendor scripts cannot be managed with NPM. I've moved them to a separate src/js/_enqueues/vendor directory and added a copy task to the Gruntfile to copy these files over to their original location.

screen shot 2017-12-19 at 14 50 16

5. Need to install WordPress packages and assign them to a global.

WordPress Packages is an initiative to extract general purpose WP libraries to separate NPM packages, so they can also be used outside of the scope of WordPress. The first example of this is wp-includes/wp-a11y.js. I've configured Webpack to automatically generate that file based on the @wordpress/a11y package. This was the chore that lead to this undertaking. Technically it's not a very hard thing to do, but I'd like to be as forward compatible as possible, so that's why I decided to zoom out a bit.

Webpack simply provides the library option for this kind of thing.

entry: {
    'src/wp-includes/js/wp-a11y.js': ['@wordpress/a11y']
},
output: {
    filename: 'src/wp-includes/js/wp-a11y.js',
    library: [ 'wp', 'a11y', 'speak' ],
}

6. Allowing a build step.

At WCUS, we pretty much agreed a build step is a necessity in modern JS development. While we can do much to help onboard new developers to get used to this, it simply makes sense to run WP from the build directory, no longer directly from source.

7. Managing packages under WP source.

At WCUS, it was pretty much decided WP packages should be managed in WP core after all. This setup can easily facilitate that. I'd propose to simply have a packages directory in src/js where we can continue managing them with lerna.


Testing

I ran a diff to compare the build that this thing generates with the current build on master:

➜  wp diff -qr compare/build mirror/build
Files compare/build/wp-admin/css/common-rtl.css and mirror/build/wp-admin/css/common-rtl.css differ
Files compare/build/wp-admin/css/common-rtl.min.css and mirror/build/wp-admin/css/common-rtl.min.css differ
Files compare/build/wp-includes/js/backbone.min.js and mirror/build/wp-includes/js/backbone.min.js differ
Files compare/build/wp-includes/js/hoverIntent.js and mirror/build/wp-includes/js/hoverIntent.js differ
Files compare/build/wp-includes/js/hoverIntent.min.js and mirror/build/wp-includes/js/hoverIntent.min.js differ
Files compare/build/wp-includes/js/imagesloaded.min.js and mirror/build/wp-includes/js/imagesloaded.min.js differ
Files compare/build/wp-includes/js/jquery/jquery-migrate.js and mirror/build/wp-includes/js/jquery/jquery-migrate.js differ
Files compare/build/wp-includes/js/jquery/jquery.color.min.js and mirror/build/wp-includes/js/jquery/jquery.color.min.js differ
Files compare/build/wp-includes/js/jquery/jquery.js and mirror/build/wp-includes/js/jquery/jquery.js differ
Files compare/build/wp-includes/js/masonry.min.js and mirror/build/wp-includes/js/masonry.min.js differ
Files compare/build/wp-includes/js/media-audiovideo.js and mirror/build/wp-includes/js/media-audiovideo.js differ
Files compare/build/wp-includes/js/media-audiovideo.min.js and mirror/build/wp-includes/js/media-audiovideo.min.js differ
Files compare/build/wp-includes/js/media-grid.js and mirror/build/wp-includes/js/media-grid.js differ
Files compare/build/wp-includes/js/media-grid.min.js and mirror/build/wp-includes/js/media-grid.min.js differ
Files compare/build/wp-includes/js/media-models.js and mirror/build/wp-includes/js/media-models.js differ
Files compare/build/wp-includes/js/media-models.min.js and mirror/build/wp-includes/js/media-models.min.js differ
Files compare/build/wp-includes/js/media-views.js and mirror/build/wp-includes/js/media-views.js differ
Files compare/build/wp-includes/js/media-views.min.js and mirror/build/wp-includes/js/media-views.min.js differ
Files compare/build/wp-includes/js/twemoji.js and mirror/build/wp-includes/js/twemoji.js differ
Files compare/build/wp-includes/js/underscore.min.js and mirror/build/wp-includes/js/underscore.min.js differ
Files compare/build/wp-includes/js/zxcvbn.min.js and mirror/build/wp-includes/js/zxcvbn.min.js differ
Files compare/build/wp-includes/version.php and mirror/build/wp-includes/version.php differ

For most of those there is a valid explanation. The media files have been slightly altered to fit the new setup. Some files are now copied over from node_modules and don't end in a newline.

herregroen commented 6 years ago

I'm currently looking at optimising the watch task in order to achieve much faster build times, previously the watch task would rebuild all JS on any javascript changes which meant around 20 seconds between changing a file and being able to access the new build.

Removing all steps that will never need to be run on a change in the `src/js/ directory gets that down to about 6 seconds but that isn't enough yet. This is mainly due to the uglify and validatejs tasks not being aware of which files changed and thus processing all files.

The watch task will need to be smarter to only run the bare minimum. I'll continue looking at ways to make this happen. I'll first take a look at webpack to see if it has any options to support the rather unique use-case we have here and otherwise see if I can supply a custom function to determine exactly which tasks need to be run on which files when a source file changes.

atimmer commented 6 years ago

@herregroen This is exactly what webpack-dev-server does. No idea if that's feasible for this PR though.

moorscode commented 6 years ago

PHPUnit fixes: https://github.com/Yoast/wordpress-develop-mirror/pull/107

omarreiss commented 6 years ago

Closing this as I had to rebase a couple of times. The most up-to-date branch is https://github.com/Yoast/wordpress-develop-mirror/tree/rebased-js-src-move/