WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 10 hours ago

#48154 reopened enhancement

Build Tools: Integrate DependencyExtractionWebpackPlugin in the JS build

Reported by: gziolo Owned by: gziolo
Milestone: 5.4 Priority: normal
Severity: normal Version: trunk
Component: Build/Test Tools Keywords: has-patch dev-feedback
Focuses: javascript Cc:
PR Number:

Description

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.

Attachments (7)

49154.diff (14.6 KB) - added by gziolo 4 months ago.
Screen Shot 2019-09-24 at 17.27.29.png (67.9 KB) - added by gziolo 4 months ago.
The structure of the js folders with asset files generated
48154-json.diff (153.4 KB) - added by gziolo 7 weeks ago.
Integration which uses JSON format as an output
48154-php.diff (155.1 KB) - added by gziolo 7 weeks ago.
Bring back PHP extensions to asset files but move to their own folder
Screen Shot 2019-12-04 at 16.10.08.png (36.2 KB) - added by gziolo 7 weeks ago.
Assets in the new location
102-asset-php-travis-green.diff (155.6 KB) - added by gziolo 7 weeks ago.
Patch updated to ensure all tests pass on Travis
48154-svn-ignore.diff (169.7 KB) - added by gziolo 6 weeks ago.
Svn ignore updated and Grunt tasks polished

Download all attachments as: .zip

Change History (29)

@gziolo
4 months ago

#1 @gziolo
4 months ago

  • Owner set to gziolo
  • Version set to trunk

@gziolo
4 months ago

The structure of the js folders with asset files generated

#2 @gziolo
4 months ago

@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.

#3 @adamsilverstein
4 months ago

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?

This ticket was mentioned in Slack in #core-js by gziolo. View the logs.


4 months ago

#5 @gziolo
7 weeks ago

  • Version changed from 5.3 to trunk

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
7 weeks ago

Integration which uses JSON format as an output

#6 @youknowriad
7 weeks ago

  • Keywords commit added

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.

#7 follow-up: @ocean90
7 weeks ago

  • Keywords commit removed

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?

This ticket was mentioned in Slack in #core-editor by gziolo. View the logs.


7 weeks ago

@gziolo
7 weeks ago

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

#9 in reply to: ↑ 7 @gziolo
7 weeks ago

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.

#10 @gziolo
7 weeks ago

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
7 weeks ago

Assets in the new location

@gziolo
7 weeks ago

Patch updated to ensure all tests pass on Travis

This ticket was mentioned in Slack in #core-js by gziolo. View the logs.


7 weeks ago

#13 @netweb
6 weeks ago

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

#14 follow-up: @gziolo
6 weeks ago

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.

#15 in reply to: ↑ 14 @netweb
6 weeks ago

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 👍🏼

This ticket was mentioned in Slack in #core-editor by gziolo. View the logs.


6 weeks ago

@gziolo
6 weeks ago

Svn ignore updated and Grunt tasks polished

#17 @gziolo
6 weeks ago

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

#18 @ocean90
5 weeks ago

  • Milestone changed from Awaiting Review to 5.4

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

#19 @gziolo
3 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#20 @azaozz
6 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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?

Last edited 6 days ago by azaozz (previous) (diff)

#21 @gziolo
19 hours ago

@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.

#22 @netweb
10 hours ago

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

Note: See TracTickets for help on using tickets.