Opened 5 years ago
Closed 4 years ago
#48154 closed defect (bug) (fixed)
Build Tools: Integrate DependencyExtractionWebpackPlugin in the JS build
Reported by: | gziolo | Owned by: | gziolo |
---|---|---|---|
Milestone: | 5.5 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | needs-patch 2nd-opinion |
Focuses: | javascript | Cc: |
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 (13)
Change History (103)
#2
@
5 years 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
@
5 years 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.
5 years ago
#5
@
5 years 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.
#6
@
5 years 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:
↓ 9
@
5 years 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.
5 years ago
#9
in reply to:
↑ 7
@
5 years 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
@
5 years ago
The changes added should be easier to review using this diff on GitHub:
I only added a few styling tweaks afterward.
#11
@
5 years ago
Travis seems to be happy: https://travis-ci.com/WordPress/wordpress-develop/builds/139670660.
This ticket was mentioned in Slack in #core-js by gziolo. View the logs.
5 years ago
#13
@
5 years 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
pathsvn 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 parentgrunt 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
orgrunt copy:assets
does not create a folder at/src/wp-includes/assets
with the expecteddist
folder and*.asset.php
files
#14
follow-up:
↓ 15
@
5 years 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 ongrunt webpack:prod
andgrunt webpack:dev
grunt copy:assets
is subtask ofgrunt assets:move
so the issue is the same/src/wp-includes/assets/dist
- this is the target fornpm run build:dev
/build/wp-includes/assets/dist
- this is the target fornpm 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
@
5 years 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 ongrunt webpack:prod
andgrunt webpack:dev
grunt copy:assets
is subtask ofgrunt assets:move
so the issue is the same/src/wp-includes/assets/dist
- this is the target fornpm run build:dev
/build/wp-includes/assets/dist
- this is the target fornpm 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.
5 years ago
#18
@
5 years 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.
#20
@
5 years 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?
#21
follow-up:
↓ 24
@
5 years 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
@
5 years 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
#23
follow-up:
↓ 26
@
5 years ago
- Component changed from Build/Test Tools to Script Loader
- Keywords needs-patch 2nd-opinion added; has-patch removed
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 tofalse
which will use the current WP version. - The addition of
assets
sub-directory inwp-includes
seems a bit misleading/confusing. The nameassets
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).
#24
in reply to:
↑ 21
;
follow-up:
↓ 25
@
5 years ago
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.
#25
in reply to:
↑ 24
@
5 years ago
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.
#26
in reply to:
↑ 23
;
follow-up:
↓ 27
@
5 years ago
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 tofalse
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 inwp-includes
seems a bit misleading/confusing. The nameassets
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
toscript-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?
#27
in reply to:
↑ 26
@
5 years ago
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.
#28
follow-up:
↓ 31
@
5 years ago
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.
#29
@
5 years ago
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.)
#31
in reply to:
↑ 28
;
follow-up:
↓ 36
@
5 years ago
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.
#32
follow-up:
↓ 33
@
5 years ago
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
#33
in reply to:
↑ 32
@
5 years ago
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.
#34
follow-up:
↓ 35
@
5 years ago
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
#35
in reply to:
↑ 34
@
5 years ago
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.
#36
in reply to:
↑ 31
@
5 years ago
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.
#37
@
5 years ago
- Keywords dev-feedback removed
- Version trunk deleted
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.
#38
@
5 years ago
@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
This ticket was mentioned in Slack in #core by david.baumwald. View the logs.
5 years ago
#40
follow-up:
↓ 41
@
5 years ago
- Keywords has-patch added; needs-patch removed
- Milestone changed from 5.4 to Future Release
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.
#41
in reply to:
↑ 40
@
5 years ago
- Keywords needs-patch added; has-patch removed
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.
#42
@
5 years ago
- Milestone changed from Future Release to 5.4
Yeah this doesn't need to be bumped, this is broken in trunk and the moment and needs to be fixed.
This ticket was mentioned in Slack in #core by antpb. View the logs.
5 years ago
#45
@
5 years ago
Follow up #49470 that looks into removing the "helper functions" added in WP 5.0 and making the list of scripts easier to maintain.
#46
follow-up:
↓ 47
@
5 years ago
@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.
#47
in reply to:
↑ 46
@
5 years ago
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 :)
#48
@
5 years ago
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.
#49
follow-up:
↓ 54
@
5 years ago
- Keywords has-patch added; needs-patch removed
@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
andBUILD_DIR
aswp-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.
This ticket was mentioned in PR #164 on WordPress/wordpress-develop by azaozz.
5 years ago
#50
tellthemachines commented on PR #164:
5 years ago
#51
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 😄
#54
in reply to:
↑ 49
;
follow-up:
↓ 55
@
5 years ago
Replying to azaozz:
In 48154.diff:
TODO/TBD: Wondering if the assets file should be committed like the rest of the PHP files inwp-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.
#55
in reply to:
↑ 54
@
5 years ago
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.
#56
@
5 years ago
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.
#59
follow-up:
↓ 61
@
5 years ago
- Keywords commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
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
.
@
5 years ago
Warnings for the non-existing included file and the following foreach invalid argument
This ticket was mentioned in Slack in #core-js by gziolo. View the logs.
5 years ago
#61
in reply to:
↑ 59
@
5 years ago
Replying to afercia:
src/wp-includes/assets
directory isn't copied over thebuild
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.
#62
@
5 years ago
Ah, I see what's going on. Dynamic clean would delete it. Fixing.
#64
@
5 years ago
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.
#65
@
5 years ago
Don't seem to be able to reproduce this. How exactly did you build WP?
For the records: npm run build
:)
#66
follow-up:
↓ 67
@
5 years ago
@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.
#67
in reply to:
↑ 66
;
follow-up:
↓ 69
@
5 years ago
- Keywords needs-patch added; has-patch removed
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.
This ticket was mentioned in Slack in #core by azaozz. View the logs.
5 years ago
#69
in reply to:
↑ 67
;
follow-up:
↓ 70
@
5 years ago
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?
#70
in reply to:
↑ 69
@
5 years ago
- Keywords 2nd-opinion added
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; }
#72
follow-up:
↓ 73
@
5 years ago
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.
#73
in reply to:
↑ 72
@
5 years ago
- Component changed from Script Loader to Build/Test Tools
- Milestone changed from 5.4 to 5.5
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 indevelopment
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 forproduction
.
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.
#74
@
5 years ago
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.
This ticket was mentioned in Slack in #core by johnbillion. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-committers by peterwilsoncc. View the logs.
5 years ago
#77
follow-up:
↓ 78
@
5 years ago
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.
#78
in reply to:
↑ 77
@
5 years ago
Replying to peterwilsoncc:
As noted above, this is currently causing uncommitted files to appear in the
src
directory when runninggrunt build --dev
orgrunt 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 ofsrc/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.
#79
follow-ups:
↓ 80
↓ 81
@
5 years ago
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.
#80
in reply to:
↑ 79
@
5 years ago
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?).
#81
in reply to:
↑ 79
;
follow-up:
↓ 82
@
5 years ago
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.
#82
in reply to:
↑ 81
@
5 years ago
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.
#83
@
5 years ago
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?
#84
@
5 years ago
Related: #49663. Should fix the reason for all the missing files when running tests.
#86
@
5 years ago
Nice, that was fast @azaozz. It starts to shape up nicely, thank you for constant effort to polish this workflow :)
The structure of the js folders with asset files generated