Build Tools: Integrate DependencyExtractionWebpackPlugin in the JS build

[gziolo]

This is a try to integrate DependencyExtractionWebpackPlugin which we battle-tested in Gutenberg.

This webpack plugin serves two purposes:

• Externalize dependencies that are available as script dependencies on modern WordPress sites.
• Add an asset file for each entry point that declares an object with the list of WordPress script dependencies for the entry point. The asset file also contains the current version calculated for the current source code.

This will greatly simplify the process of upgrading npm packages which change after every Gutenberg release. It might even useful during the WordPress release cycle as we might need to publish more often as we discover critical bugs and regressions.

Code also available at ​https://github.com/WordPress/wordpress-develop/pull/102 where I test its integration with Travis.

[gziolo]

The structure of the js folders with asset files generated

[gziolo]

@adamsilverstein @omarreiss this adds several PHP files for each JS script. Is it fine to keep them next to distribution version of JS files as presented on the screenshot above?

@schlessera raised it as a potential issue in ​the comment under original PR on GitHub related to Gutenberg integration:

In a plugin I'm working on, for example, this now causes PHP files to reside under assets/js, which is totally misleading. This is the type of folder that is usually sent to a CDN, not executed on the server.

[adamsilverstein]

Hey @gziolo thanks for working on this. Can you explain a bit more how will/are these PHP files be used?

The CDN concern does seem valid. Also, it feels generally confusing/wrong to store PHP files in a folder named js explicitly because it is full of JavaScript files.

I checked the generated php files and see they return the dependencies and a version key. Can we store them in a separate location?

[gziolo]

It also looks like JSON is now required to run WordPress:
​https://make.wordpress.org/core/2019/10/15/php-native-json-extension-now-required/

I'm inclined to change the output format to JSON for WordPress core which should make this non-issue, ale the information stored in JSON file are extracted from JS file so there are no security risks at all.

[gziolo]

Integration which uses JSON format as an output

[youknowriad]

I'm really excited about this change and the potential it has to simplify our packages update in Core. Let's land this in trunk to figure out any shortcomings we might have missed asap.

[ocean90]

Is it possible to merge the data into one file? I'm a bit concerned about the increased file reads + json_decode(). PHP files seem to provide the benefit of caching with OPcache?

[gziolo]

Bring back PHP extensions to asset files but move to their own folder

[gziolo]

Replying to ocean90:

Is it possible to merge the data into one file? I'm a bit concerned about the increased file reads + json_decode(). PHP files seem to provide the benefit of caching with OPcache?

I don't think we want to merge all files into one. I refactored this patch to use PHP files instead. The difference is that now Grunt moves those files to its own folder at the same level in the hierarchy as js to ensure that they won't get exposed together with static assets.

[gziolo]

The changes added should be easier to review using this diff on GitHub:

​https://github.com/WordPress/wordpress-develop/pull/102/commits/af136b1ee3f2099c873a0044d4cdd995ae8b1ccb

I only added a few styling tweaks afterward.

[gziolo]

Assets in the new location

[gziolo]

Patch updated to ensure all tests pass on Travis

[gziolo]

Travis seems to be happy: ​https://travis-ci.com/WordPress/wordpress-develop/builds/139670660.

[netweb]

Notes:

• Prior to committing the package-lock.json file requires refreshing, otherwise patch applies cleanly:

  patching file package-lock.json
  Hunk #35 FAILED at 6101.
  Hunk #36 FAILED at 6114.
  Hunk #37 FAILED at 6174.
  Hunk #39 FAILED at 7055.
  Hunk #53 FAILED at 8404.
  Hunk #79 FAILED at 17535.
  Hunk #82 FAILED at 18504.
  7 out of 95 hunks FAILED -- saving rejects to file package-lock.json.rej
  

• p.s No objections to sneaking in some npm audit fixes during commit either

---

• The svn:ignore file also requires updating for the /src/wp-includes/assets path

  svn propget svn:ignore
  # Configuration files with possibly sensitive information
  wp-config.php
  wp-tests-config.php
  .htaccess
  # Files and folders related to build/test tools
  phpunit.xml
  phpcs.xml
  .phpcs.xml
  node_modules
  npm-debug.log
  build
  wp-cli.local.yml
  .git
  jsdoc
  vendor
  docker-compose.override.yml
  

---

Grunt

• The clean:assets task is not included in the parent grunt clean task, should it?

• Based on the image above the php assets are now stored in the /src/wp-includes/assets/dist folder, added a comment to the GH PR ​here

• Running either grunt assets:move or grunt copy:assets does not create a folder at /src/wp-includes/assets with the expected dist folder and *.asset.php files

[gziolo]

The clean:assets task is not included in the parent grunt clean task, should it?

Good catch. I think I missed it. We can update it.

svn:ignore

Yes, I copied the diff from GitHub. I will create next patch from svn.

@netweb, thanks for leaving feedback. I think I need to clarify a few things:

• grunt assets:move depends on grunt webpack:prod and grunt webpack:dev
• grunt copy:assets is subtask of grunt assets:move so the issue is the same
• /src/wp-includes/assets/dist - this is the target for npm run build:dev
• /build/wp-includes/assets/dist - this is the target for npm run build

In general, this is very hard to tackle if we want to use PHP files because webpack natively isn't flexible enough to easily allow moving assets to a different folder.

[netweb]

Replying to gziolo:

Good catch. I think I missed it. We can update it.

svn:ignore

Yes, I copied the diff from GitHub. I will create next patch from svn.

It needs to be in both, the svn:ignore property and the .gitignore

@netweb, thanks for leaving feedback. I think I need to clarify a few things:

• grunt assets:move depends on grunt webpack:prod and grunt webpack:dev
• grunt copy:assets is subtask of grunt assets:move so the issue is the same
• /src/wp-includes/assets/dist - this is the target for npm run build:dev
• /build/wp-includes/assets/dist - this is the target for npm run build

In general, this is very hard to tackle if we want to use PHP files because webpack natively isn't flexible enough to easily allow moving assets to a different folder.

Thanks, let me test that all again 👍🏼

[gziolo]

Svn ignore updated and Grunt tasks polished

[gziolo]

@ocean90 and @netweb - this if ready for another review.

[ocean90]

48154-svn-ignore.diff​ looks good to me. The svn ignore prop needs to be added on commit.

[gziolo]

In 47035:

Build Tools: Integrate DependencyExtractionWebpackPlugin in the JS build.

This patch integrates DependencyExtractionWebpackPlugin which was battle-tested in Gutenberg.

This will greatly simplify the process of upgrading npm packages which change after every Gutenberg release. It might even useful during the WordPress release cycle as we might need to publish more often as we discover critical bugs and regressions.

Props jonsurrell, adamsilverstein, youknowriad, ocean90, netweb.

Fixes #48154.

[azaozz]

May be missing something but this still looks a bit broken, even after #49144. Running npm run build followed by phpunit throws lots of missing files warnings (presumably because the new assets dir is only in /build, but phpunit looks in /src).

Not sure what's the best fix. Should phpunit test /build by default, or maybe these includes should be conditional on running from /build?

Also, does it make sense to distribute these files with the build/production version of WP? What purpose do they serve in production?

[gziolo]

@azaozz - what kind of warnings do you see? It should ignore all missing files as you pointed out after #49144 landed.

I have a related question to that. Is there any clean way to leave feedback to the developer when those files are missing which doesn't break unit tests? It would be great for debugging purposes. @jorgefilipecosta had some issues with the missing package which wasn't listed in the core but was added in Gutenberg lately.

[netweb]

A couple of other users have noticed similar issues and reported them in Slack ​here, I've not had the tine to dig any deeper into this unfortunately

[azaozz]

Looking more at the changes here, this is set as "Build/Test Tools" but in fact enhances/partially refactors how script-loader.php works. That's (mostly) OK when in /src, but perhaps not that good for /build (in production). If this was intended only for the source, this ticket need more work :)

As this changes script-loader there are few more considerations:

• It's not a good idea to have a bunch of (around 80) files be included on each WP run. It's true, include() is pretty fast in PHP, but what's the purpose of including these files separately in production? What is fixed or enhanced by that? These files can be present in /src if need be, but should not exist in /build.
• In script-loader the version string is only used to "bust cache" when a js or css file changes between different WP versions. Having separate "cache-busting" strings for *.js and *.min.js doesn't make sense. In addition the current "cache-busting" strings seem needlessly long. Ideally the current WP version would be used when adding or changing a js or css file. This also removes the expectation that a particular file's version can be retrieved by looking at script-loader (that is incorrect in many cases). Alternatively the "cache-busting" (version) should be set to false which will use the current WP version.
• The addition of assets sub-directory in wp-includes seems a bit misleading/confusing. The name assets is widely used on the web, with different context. Best to rename it imho.

Changing the component to script-loader as this is a refactoring/enhancement there. This will require "merging" all one-line files from wp-includes/assets/dist to script-loader.php when building (perhaps similarly to how the emoji code is handled).

[azaozz]

Replying to gziolo:

@azaozz - what kind of warnings do you see?

A big bunch of PHP Warning: include(.../src/wp-includes/assets/dist/[name].php): failed to open stream: No such file or directory in ...\src\wp-includes\script-loader.php on line 295 (that's before the phpunit tests start).

I have a related question to that. Is there any clean way to leave feedback to the developer when those files are missing which doesn't break unit tests? It would be great for debugging purposes. @jorgefilipecosta had some issues with the missing package which wasn't listed in the core but was added in Gutenberg lately.

I may be misunderstanding but wouldn't that throw a js error or show a 404 in the browser console?

Generally thinking that splitting these files is not a great idea. I understand that it makes it somewhat easier to build, but the "building tools" are to make things better/easier, not to add more complexity :) Integrating the DependencyExtractionWebpackPlugin shouldn't be changing how the (production) code works, it should be making building easier and keeping the code working in the most efficient/best way.

In that terms these 86 files may be a "temporary build step", but seems it would be best to add the strings from them to script-loader.php in the next step, and delete them. This seems pretty easy to accomplish in Grunt.

[gziolo]

Replying to azaozz

A big bunch of PHP Warning: include(.../src/wp-includes/assets/dist/[name].php): failed to open stream: No such file or directory in ...\src\wp-includes\script-loader.php on line 295 (that's before the phpunit tests start).

The only case when it would happen is when someone run npm run build:dev in the past and they have JS files generated. For the fresh build, it should never be the case because the asset file is generated together with the matching JS file.

I may be misunderstanding but wouldn't that throw a js error or show a 404 in the browser console?

No, it doesn't throw. When the script dependencies are missing then you only see the blank page. It's a general issue with how scripts are handled in core.

Generally thinking that splitting these files is not a great idea. I understand that it makes it somewhat easier to build, but the "building tools" are to make things better/easier, not to add more complexity :) Integrating the DependencyExtractionWebpackPlugin shouldn't be changing how the (production) code works, it should be making building easier and keeping the code working in the most efficient/best way.
In that terms these 86 files may be a "temporary build step", but seems it would be best to add the strings from them to script-loader.php in the next step, and delete them. This seems pretty easy to accomplish in Grunt.

Feel free to open a patch to refactor the existing setup in core. It seems like a great idea. Webpack can produce also JSON files if that would make it easier to process in such a scenario.

[gziolo]

Replying to azaozz:

Looking more at the changes here, this is set as "Build/Test Tools" but in fact enhances/partially refactors how script-loader.php works. That's (mostly) OK when in /src, but perhaps not that good for /build (in production). If this was intended only for the source, this ticket need more work :)

• It's not a good idea to have a bunch of (around 80) files be included on each WP run. It's true, include() is pretty fast in PHP, but what's the purpose of including these files separately in production? What is fixed or enhanced by that? These files can be present in /src if need be, but should not exist in /build.

You either load development or production version so it's half of the number you shared. I don't think I understand the concern you raise. It works exactly the same as all JavaScript files generated in the build step. Those asset files list all script dependencies which otherwise would have to be maintained manually. If we were to change how the /build is handled, it would have to change for JS files as well.

• In script-loader the version string is only used to "bust cache" when a js or css file changes between different WP versions. Having separate "cache-busting" strings for *.js and *.min.js doesn't make sense. In addition the current "cache-busting" strings seem needlessly long. Ideally the current WP version would be used when adding or changing a js or css file. This also removes the expectation that a particular file's version can be retrieved by looking at script-loader (that is incorrect in many cases). Alternatively the "cache-busting" (version) should be set to false which will use the current WP version.

*.js and *.min.js have different content so they have different hash assigned. The way it works it allows for reusing the same cached file in the browser when its content doesn't change. It's a quite popular technique for ensuring that you never get a cached version when the content of the file changed.

• The addition of assets sub-directory in wp-includes seems a bit misleading/confusing. The name assets is widely used on the web, with different context. Best to rename it imho.

What do you propose as an alternative?

Changing the component to script-loader as this is a refactoring/enhancement there. This will require "merging" all one-line files from wp-includes/assets/dist to script-loader.php when building (perhaps similarly to how the emoji code is handled).

Is it really a concern to have multiple include files these days with all caching mechanism that PHP have baked in?

[azaozz]

Replying to gziolo:

I understand the concern you raise. It works exactly the same as all JavaScript files generated in the build step. Those asset files list all script dependencies which otherwise would have to be maintained manually.

The main concern here is that this is an "architectural change" in the (built) application that is done to accommodate how some building tool(s) work and doesn't make sense for the application.

Why would you want to split one file into 80 one-line files and then require them (or half of them) every time the app runs? There is some mild slowdown/increased use of resources on the server regardless of whether these files are used or not. What are the benefits for the built app?

Then, making architectural changes/adding new files "locks" them in place. Generally it becomes quite hard to move or delete them later as these changes are in the built/production version.

If we were to change how the /build is handled, it would have to change for JS files as well.

Sure, lets look at that too. IMHO it makes sense to optimize while building not "deoptimize" :)

*.js and *.min.js have different content so they have different hash assigned. The way it works it allows for reusing the same cached file in the browser when its content doesn't change.

Right. The content (code) is formatted differently but as code it's exactly the same. However the file names are different. Switching from *.js to *.min.js will never use the non-minified version of the file. Since the code is exactly the same it seems to make sense to use the same "cache busting" string for both versions of the file. In reality it doesn't really matter, as long as there are new cache busting strings in new versions of WP.

It's a quite popular technique for ensuring that you never get a cached version when the content of the file changed.

Hehe, yeah, that's the old old way to (attempt to) refresh network and browser caches. It still may fail in some cases, mostly for network caches, after all these years...

Generally it doesn't matter what string is used. However using a "real" version or hash may eventually bring problems with some plugins expecting to be able to determine a particular dependency's version by using the cache busting string. This shouldn't be encouraged, if possible. Thinking that passing false may be a better option there, then the current WP version will be used. Or perhaps we can "hard-code" the current WP version at the time the file was last updated in core.

Is it really a concern to have multiple include files these days with all caching mechanism that PHP have baked in?

It's not "huge" concern, but it still uses server resources. With WordPress' "footprint" that is still quite a lot. Thinking that a good "rule of thumb" would be to avoid that when there are no benefits for the (built) app.

[johnbillion]

I'm seeing 86 warnings related to this change on my local dev site which is running from src.

Warning	include(/sites/wp/src/wp-includes/assets/dist/a11y.asset.php): failed to open stream: No such file or directory in wp-includes/script-loader.php:296
Warning	include(): Failed opening '/sites/wp/src/wp-includes/assets/dist/a11y.asset.php' for inclusion (include_path='.:/usr/local/Cellar/php@7.3/7.3.13/share/php@7.3/pear') in wp-includes/script-loader.php:296

Repeated for each of these *.asset.php files.

[johnbillion]

Regarding the above points about performance, can we get some before and after performance metrics please? (Once the issue that's causing the missing files has been fixed.)

[ocean90]

#49311 was marked as a duplicate.

[ocean90]

Replying to johnbillion:

I'm seeing 86 warnings related to this change on my local dev site which is running from src.

Looks like the --dev flag is not handled? Related: #44492.

[pbearne]

The code has a test for file exist for the js but not for the PHP file so if the npm ​build:dev or main build has not been run correctly it errors ( as seen in the errors above)

I wonder if it would not be better to test for the PHP file and not the JS as the PHP file is created if the JS is there ( this is less expensive than testing for both )

We should/could throw an error message if we are missing a PHP file instructing the DEV to run npm ​build:dev

[gziolo]

Replying to pbearne:

I wonder if it would not be better to test for the PHP file and not the JS as the PHP file is created if the JS is there ( this is less expensive than testing for both )

Yes, it makes a lot of sense. The assumption made previously didn't take into account that existing WordPress development codebases have JS files already. It's better to verify whether the PHP file exists. Your patch proposed in #49311 addresses it properly. Thank you for taking care of it. I'd personally be fine with landing it after review and testing.

We should/could throw an error message if we are missing a PHP file instructing the DEV to run npm ​build:dev

I asked about that in one of my comments above. It would be the preferred approach but I don't know how to do it to don't make PHPUnit tests fail when they don't run npm run build:dev. In many projects, the expectation is that you don't to build JS files to run those tests which makes a lot of sense.

[pbearne]

The patch in #49311 adds an additional file_exists() test for the PHP files and sets defaults for if a file is missing

The question is can assume that the JS exists if the find the PHP file and only have one test not two as a performance saving

[gziolo]

Replying to pbearne:

The patch in #49311 adds an additional file_exists() test for the PHP files and sets defaults for if a file is missing

The question is can assume that the JS exists if the find the PHP file and only have one test not two as a performance saving

PHP file is created by Webpack in the same task which output JS build file. A separate task moves PHP asset files to a different location. In the previous implementation (before PHP assets were introduced), there was no check for the existence of JS file. It wasn't raised as an issue for nearly 2 years so I assume you could skip the check for JS file as it must be handled in the script registration logic. It needs to be confirmed though.

[gziolo]

Ensures that no warnings are displayed when no asset file found but JS file exists

[gziolo]

Replying to ocean90:

Looks like the --dev flag is not handled? Related: #44492.

I wasn't aware of this flag. I'll look into it.

I added a patch based on the proposal from @pbearne that should fix the issue with warnings. I tested by removing generated assets folders and running npm run test:php. This is at least how I was able to reproduce it. Let's land it and discuss further steps.

[johnbillion]

This seems to be papering over the issue. If the a PHP asset file is missing, then enqueing the asset without its dependencies isn't correct.

If an asset file is missing this should trigger a wp_die() message similar to the one shown when a fresh installation is used but the build command has yet to be run.

[pbearne]

added wp_die example change text as needed

[pbearne]

@johnbillion I agree

We can drop the file test for JS file and just error if a PHP file is missing
Just pushed a quick patch with example code of what I have in mind

[davidbaumwald]

This ticket still needs a decision, and with 5.4 Beta 1 landing tomorrow, this is being moved to Future Release. If any maintainer or committer feels this can be included in 5.4 or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

[azaozz]

Replying to davidbaumwald:

with 5.4 Beta 1 landing tomorrow, this is being moved to Future Release.

Why "future release"? This is mostly a build tools problem introduced during the 5.4 development stage (alpha). Would be great to get it fixed, and beta is a good time for it :)

As far as I see the problem here is that the DependencyExtractionWebpackPlugin is not doing a good job when used to build PHP files, it needs a bit more.

[johnbillion]

Yeah this doesn't need to be bumped, this is broken in trunk and the moment and needs to be fixed.

[azaozz]

Follow up #49470 that looks into removing the "helper functions" added in WP 5.0 and making the list of scripts easier to maintain.

[gziolo]

@azaozz, I started ​https://github.com/WordPress/gutenberg/pull/20330 which should make it possible to combine all asset files into one. The biggest challenge is how to generate keys for individual entry points. I think that path generated for all build files, that you need to use when registering them with WordPress, makes the most sense.

[azaozz]

Replying to gziolo:

Yep, that looks a lot better, thanks for working on it! Trying to figure out how to get webpack to always emit the file (both in prod and dev) so grunt can just append it inside script-loader.php. Will try few more ways/tweaks tomorrow :)

[gziolo]

I published ​https://www.npmjs.com/package/@wordpress/dependency-extraction-webpack-plugin with the new combineAssets flag. Let me know how can I help with integration on the core side.

[azaozz]

@gziolo yes, seems to be working well. Thanks! If you have a chance perhaps give it a try.

In 48154.diff:

• Update dependency-extraction-webpack-plugin to 2.3.0.
• Set it to output a single file, then copy that file to both SOURCE_DIR and BUILD_DIR as wp-includes/assets/script-loader-deps.php.
• Tweak wp_default_packages_scripts() to include and use the new file.

This seems to be working here. Running only npm run build followed by phpunit, running grunt directly, new/clean checkout of trunk, etc. Please test! :)

TODO/TBD: Wondering if the assets file should be committed like the rest of the PHP files in wp-includes/blocks? Any other ideas/tweaks?

For the record: Tried to use grunt includes to "concatenate" the assets file directly in script-loader.php. It works great for /build, but works only once for /src (as script-loader is modified). That modification can be committed, but after the next packages update will have to be reverted manually to get the new assets again... Can probably play with this a bit and get it to work, but the way it works currently seems cleaner and simpler.

Tested this with both build and build:src and it's working well. No changes to assets browser-side. Code looks good, nice to have that huge list of dependencies cleaned up 😄

[gziolo]

Replying to azaozz:

In 48154.diff:
TODO/TBD: Wondering if the assets file should be committed like the rest of the PHP files in wp-includes/blocks? Any other ideas/tweaks?

Yes, I think we should do it now that we have only one file that can change. It would solve all the issues discussed above because all asset definitions are always there. In fact, it's how PHP files for blocks are handled so it would align nicely. We just need to remove svn ignore for the folder. In general, this file would only change when packages get updated.

For the record: Tried to use grunt includes to "concatenate" the assets file directly in script-loader.php. It works great for /build, but works only once for /src (as script-loader is modified). That modification can be committed, but after the next packages update will have to be reverted manually to get the new assets again... Can probably play with this a bit and get it to work, but the way it works currently seems cleaner and simpler.

It's fine to have one file to include that contains this large array. It might even be cleaner because it's machine-generated thus hard to read for reviewers.

<?php
$assets   = include ABSPATH . WPINC . '/assets/script-loader-deps.php';
$packages = array_keys( $assets );

I like this part the most that allows removing the list of packages which was maintained manually so far. In fact, it's less error-prone because it is generated by webpack so we have now a single source of truth. I think we can also remove the list of packages that have translations and perform a simple check whether wp-18n is listed as a dependency instead.

It would be great to commit this patch to have it included in Beta 3 so feel free to land as-is and we can handle the rest later.

[azaozz]

Replying to gziolo:

TODO/TBD: Wondering if the assets file should be committed

Yes, I think we should do it now that we have only one file that can change.

OK, will do.

$packages = array_keys( $assets );

I like this part the most that allows removing the list of packages which was maintained manually so far.

Yep, much better than having to do it manually (it doesn't even need array_keys(), can do the same inside the loop).

I think we can also remove the list of packages that have translations and perform a simple check whether wp-18n is listed as a dependency instead.

Yes, would be better. Looked there a bit, three more packages have wp-18n as dependency: annotations, media-utils, and server-side-render.

It would be great to commit this patch to have it included in Beta 3 so feel free to land as-is and we can handle the rest later.

Of course. Will update the patch shortly, then go through it once more tomorrow and commit.

[azaozz]

48154.2.diff implement the changes discussed above:

• "Unignore" wp-includes/assets dir. That means the script-loader-packages.php file will be committed.
• Clean up the loop in wp_default_packages_scripts(), remove the hard-coded $package_translations list and add translations based on whether the package has 'wp-i18n' as dependency.
• Rename the assets file to script-loader-packages.php (props ocean90).

This should be ready to commit. The only thing that's different from the previous patch (apart from cleanup) is the way translations are added.

[azaozz]

In 47352:

Build Tools:

• Update the DependencyExtractionWebpackPlugin to 2.3.0 and set it to output a single assets file.
• Grunt: copy the assets file to both SOURCE_DIR and BUILD_DIR as wp-includes/assets/script-loader-packages.php.
• "Unignore" the wp-includes/assets directory. Its content will be committed similarly to wp-includes/blocks.
• Update wp_default_packages_scripts() to use the above file. This also removes the hard-coded lists of packages and packages with translations.

Props gziolo, pbearne, johnbillion, isabel_brison, ocean90, azaozz.
Fixes #48154.

Done: https://core.trac.wordpress.org/changeset/47352.

[afercia]

Looking at [47352] the whole src/wp-includes/assets directory isn't copied over the build directory during the build. Thus, the include statement will throw a Warning for all of us who run WordPress (and run the tests) from build.

[afercia]

Warnings for the non-existing included file and the following foreach invalid argument

[azaozz]

Replying to afercia:

src/wp-includes/assets directory isn't copied over the build directory during the build.

Right, it is not copied from /src, it is created every time when building, here: https://core.trac.wordpress.org/browser/trunk/Gruntfile.js#L358. This ensures the dependencies are always "fresh" and coming from Webpack, same as the js files.

Don't seem to be able to reproduce this. How exactly did you build WP? What command did you use? I suppose the directory can be copied from /src and then overwritten after Webpack outputs the assets file, but better to get to the bottom of this first.

[azaozz]

Ah, I see what's going on. Dynamic cleaning of js would delete it. Fixing.

[azaozz]

In 47380:

Build Tools: Do not delete the (now committed) script-loader-packages.php file when running clean:js and copy it to /build. Fixes cases when running grunt watch or -dev where that file may be deleted and not recreated.

See #48154.

[azaozz]

Looking a bit further, instead of moving script-loader-packages.php with Grunt, it can be outputted in wp-includes/assets by Webpack. This will need another small update of the DependencyExtractionWebpackPlugin to accept a path param when combineAssets is true.

Will make a test patch with a local copy of DependencyExtractionWebpackPlugin.

[afercia]

Don't seem to be able to reproduce this. How exactly did you build WP?

For the records: npm run build :)

[david.binda]

@azaozz it looks like the r47380 did not really make it properly to the build repository due to unresolved merge conflicts. See ​https://build.trac.wordpress.org/changeset/47167/ which introduces .mine .r47377 and .r47376 versions of the wp-includes/assets/script-loader-packages.php file, but does not update the file itself.

[azaozz]

Replying to david.binda:

looks like the r47380 did not really make it properly to the build repository

Thanks for the ping. Yeah, good that it didn't happen before beta3. Happened because /build is ignored when building locally, but is under version control in the build repo.

@dd32 @762e5e74 what would be the best way to fix this? Looking at Grunt, it cleans (empties) the /build dir every time before copying the files on every build, but that doesn't seem to work in the build repo. Shall I try to revert then delete these files directly in the build repo or is there a better way to do this?

Looking at why this happened: script-loader-packages.php is slightly different depending on whether Webpack was run in "build" or "dev" mode. My theory is that:

• /src/wp-includes/assets/script-loader-packages.php was different than what's in the build repo.
• After copying the WP files (which happens first when building) there were uncommitted changes in that file.
• Then the assets.php file was generated again by Webpack and copied to /build/wp-includes/assets/script-loader-packages.php. However this version of the file was different than the file that was copied earlier as Webpack ran in "build" mode. So SVN "saw" a file with uncommitted changes being replaced by a file with more/other changes and went into conflict resolution mode.

Seems this will happen again next time the WP packages are updated. Fixing.

[SergeyBiryukov]

Replying to azaozz:

Shall I try to revert then delete these files directly in the build repo or is there a better way to do this?

Committing to build repo directly has been done in the past, in ​https://build.trac.wordpress.org/changeset/35925 for example. That said, I'm not quite sure a merge conflict like this won't happen again in the future. If the file is different based on whether it's created in "build" or "dev" mode, should it really be in SVN?

[azaozz]

Replying to SergeyBiryukov:

If the file is different based on whether it's created in "build" or "dev" mode, should it really be in SVN?

Yeah, been trying to wrap my head around this for a while... Seems I was incorrect about how this happened. Looking again, the conflict happened in [47377] and the three extra files were created in the develop repo. Then they were copied to the build repo in [47380] when I removed the copy restriction in Grunt.

"Unversioning" script-loader-packages.php and setting wp-includes/assets back to "ignored" will prevent merge conflicts like this one in the future, but will bring back the above mentioned build problems: the file is missing in /build when running npm run dev or grunt watch, and in /src when running npm run build or grunt build, etc.

This seems to mostly affect running the unit tests after npm run build. Wondering if adding a "doing it wrong" when that file is missing would be a good way to handle this error. Perhaps something like:

if ( ! file_exists( $assets_file ) ) {
    $message = __( 'It apperas that WordPress was not build properly or is running from the wrong directory. The <code>wp-includes/assets/script-loader-packages.php</code> file is missing.' );
    _doing_it_wrong( __FUNCTION__, $message, '5.4.0' );
    return;
}

[azaozz]

In 47407:

Build Tools: Exclude the src/wp-includes/assets/ directory from copying when building. Its content is generated by Webpack and copied in another task.

See #48154.

[gziolo]

I think I have an idea of how it all can be solved. The issue we encounter with the assets file generated is caused by the difference in version hashes generated for chunks. It works this way by design because it is calculated based on the content of the output files. In production mode you usually will have a different code than in development mode because we often branch code depending on the env (we extensively use development tools, logging to JS console in dev mode, etc).

We already use different file names depending on env, see:
​https://github.com/WordPress/wordpress-develop/blob/master/tools/webpack/packages.js#L60-L63

We should extend it for combined asset files and have one for development and one for production. They would be loaded conditionally, similar to how assets are registered. The only blocker is that the name for the output file is hardcoded in the webpack plugin so we need to allow some customization as proposed by @azaozz in ​https://github.com/WordPress/gutenberg/issues/20562.

[azaozz]

Replying to gziolo:

I think I have an idea of how it all can be solved. The issue we encounter with the assets file generated is caused by the difference in version hashes generated for chunks. It works this way by design because it is calculated based on the content of the output files. In production mode you usually will have a different code than in development mode because we often branch code depending on the env (we extensively use development tools, logging to JS console in dev mode, etc).

Yeah, for now mostly the version strings (the hashes) are different, but some packages might have different dependencies too. And that may be more common in the future.

We should extend it for combined asset files and have one for development and one for production.

This is pretty much how it works now, but is not very straightforward, it's hard to "see" it. The dev version of script-loader-packages.php is in /src and is committed. The production version is generated every time npm run build (or grunt build) is run, and is in /build.

They would be loaded conditionally, similar to how assets are registered. The only blocker is that the name for the output file is hardcoded in the webpack plugin so we need to allow some customization as proposed by @azaozz in ​https://github.com/WordPress/gutenberg/issues/20562.

Right, we can change the name depending on whether Webpack runs in production or development mode, and output the file to the right place.

This ticket will need some more work (and testing). As it is for "build tools" at this point, and there is a lot of background in the comments, lets continue during 5.5.

[azaozz]

Another TODO here: building needs to ensure the packages in node_modules are up to date, and fail if they are not. That will prevent errors like in comment 65 above.

[peterwilsoncc]

As noted above, this is currently causing uncommitted files to appear in the src directory when running grunt build --dev or grunt watch --dev.

Reading through Gruntfile.js, it appears the intent is to manage the entirety of src/wp-includes/assets/ by the build process.

As such, I intend to add src/wp-includes/assets/ to the SVN ignore prop and .gitignore to both trunk and the 5.4 branch. .gitignore patch incoming.

[azaozz]

Replying to peterwilsoncc:

As noted above, this is currently causing uncommitted files to appear in the src directory when running grunt build --dev or grunt watch --dev.

Think this happened only once after script-loader-packages.php was updated and committed manually. Have you seen this happen again? Can it be reproduced consistently?

Reading through Gruntfile.js, it appears the intent is to manage the entirety of src/wp-includes/assets/ by the build process.

As such, I intend to add src/wp-includes/assets/ to the SVN ignore prop and .gitignore to both trunk and the 5.4 branch. .gitignore patch incoming.

This will bring back the previous bug (non-existing file) when building to /build and running tests on /src, and similar.

[peterwilsoncc]

Think this happened only once after script-loader-packages.php was updated and committed manually. Have you seen this happen again? Can it be reproduced consistently?

Mainly seeing it switching branch to branch as I review code on repos I can't commit too.

This will bring back the previous bug (non-existing file) when building to /build and running tests on /src, and similar.

In 48154-update-file-check.diff​ I've modified the file looked for to determine if a site has been built when running from the WordPress-develop repo.

Without wp-includes/assets/script-loader-packages.php, the JavaScript can not be run (no dependency array) and as it's generated during the build processes the remaining instructions in /src/index.php are enough.

[azaozz]

Replying to peterwilsoncc:

Mainly seeing it switching branch to branch

Right, managed to reproduce it too when "updating" to older revisions.

Without wp-includes/assets/script-loader-packages.php, the JavaScript can not be run (no dependency array) and as it's generated during the build processes the remaining instructions in /src/index.php are enough.

Yes, makes sense to test for the existence of script-loader-packages.php, 48154-update-file-check.diff looks good. Then ignoring the wp-includes/assets directory should fix the above bugs and show a proper error message when WP is not build properly.

Also thinking it can perhaps do a Grunt clean of /src/wp-includes/assets/** before running Webpack in dev mode. This will help by deleting left-over old and unversioned files there (not strictly necessary but nice to have?).

[gziolo]

Replying to peterwilsoncc:

In 48154-update-file-check.diff​ I've modified the file looked for to determine if a site has been built when running from the WordPress-develop repo.

Without wp-includes/assets/script-loader-packages.php, the JavaScript can not be run (no dependency array) and as it's generated during the build processes the remaining instructions in /src/index.php are enough.

It's a great idea to update code this way if you decide to ignore the wp-includes/assets folder again.

Also thinking it can perhaps do a Grunt clean of /src/wp-includes/assets/** before running Webpack in dev mode. This will help by deleting left-over old and unversioned files there (not strictly necessary but nice to have?).

This proposal from @azaozz is also worth considering to ensure that all files generated earlier get removed. It would also accommodate future changes that we discussed like having one asset file for development and one for production.

There is one consideration to take into account that might actually be covered with the updated check that detects whether JS build was executed. We need to ensure that PHPUnit tests pass without JS build files.

[peterwilsoncc]

Replying to gziolo:

There is one consideration to take into account that might actually be covered with the updated check that detects whether JS build was executed. We need to ensure that PHPUnit tests pass without JS build files.

I've created #49635 to follow up the discussion, I think it's safe to ignore the problem for now and consider back-porting if a suitable solution can be found.

[azaozz]

To ignore /src/wp-includes/assets again it will have to be SVN deleted, and added to the ignored props on the parent dir, /src/wp-includes. Then, when building, it will be created again but will not be committed.

One problem with version control is that once a file is added and committed, it never really goes away. It may be deleted in the current revision, but as soon as you restore a previous revision it is created again...

When that happens, if an unversioned file already exists at the same location, it will cause conflict.

The only way to prevent that is to start "fresh". Thinking won't need to rename the directory but would need to rename the file.

We need to ensure that PHPUnit tests pass without JS build files.

As far as I see the only way to do that is to add a file_exists() in script-loader.php. With 48154-update-file-check.diff it could be a "silent check" as it will never be reached when running WP, only when testing. On the other hand, can still throw "doing it wrong" user-notice perhaps?

[azaozz]

Related: #49663. Should fix the reason for all the missing files when running tests.

[azaozz]

In 47471:

Build Tools: Use the new combinedOutputFile setting for the updated DependencyExtractionWebpackPlugin and output script-loader-packages.php directly in wp-includes/assets/.

Props gziolo, azaozz.
See: #48154.

[gziolo]

Nice, that was fast @azaozz. It starts to shape up nicely, thank you for constant effort to polish this workflow :)

[SergeyBiryukov]

In 47544:

Build/Test Tools: Remove a starting empty line from svn:ignore property on wp-includes.

Follow-up to [47352].

See #48154.

[gziolo]

@azaozz anything left here? It seems to work pretty well.

[azaozz]

Right, seems to be working well. There are couple of possible TBD/TODO but they are in follow-up tickets.