New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config: Mirror packages dependencies registration with core #11637

Merged
merged 7 commits into from Nov 19, 2018

Conversation

@gziolo
Member

gziolo commented Nov 8, 2018

Description

This PR tries to mirror the setup which we have in WordPress core for packages dependencies in PHP part of Gutenberg. Related code:

https://github.com/WordPress/wordpress-develop/blob/5504866f2098d15a7bc7d270dd12383b9a579884/src/wp-includes/script-loader.php#L168-L366

It goes even further and moved the list of packages and their dependencies to their own file to make it even easier to update in WordPress core.

In addition, this PR removes the hardcoded list of Gutenberg packages from webpack.config.js. We can reason it based on the list of dependencies included in package.json file.

The very last step (definitely a separate PR) would be to generate this PHP array with dependencies using some automated tooling so we didn't have to maintain it manually anymore. We are not quite there as we still have some WordPress core dependencies listed which aren't published to this repository and there not published to npm.

'wp-escape-html',
gutenberg_url( 'build/escape-html/index.js' ),
array( 'wp-polyfill' ),
filemtime( gutenberg_dir_path() . 'build/element/index.js' ),

This comment has been minimized.

@gziolo

gziolo Nov 8, 2018

Member

bug :)

@gziolo gziolo requested a review from mcsf Nov 8, 2018

*/
function gutenberg_register_packages_scripts() {
$packages_dependencies = array(
'api-fetch' => array( 'wp-polyfill', 'wp-hooks', 'wp-i18n', 'wp-url' ),

This comment has been minimized.

@gziolo

gziolo Nov 8, 2018

Member

@aduth - could we assume that all scripts require wp-polyfill and inject it in the loop?

This comment has been minimized.

@aduth

aduth Nov 8, 2018

Member

We can assume for all Gutenberg-related core scripts, yes.

This comment has been minimized.

@gziolo

gziolo Nov 16, 2018

Member

Awesome, I took that into account 👍

This comment has been minimized.

@aduth

aduth Nov 16, 2018

Member

There's a bit of me which is sad in losing the explicitness, at least in considering package-dependencies.php as an exhaustive set. I suppose it is more an implicit thing if we're making a blanket statement that all new scripts should define wp-polyfill as a dependency.

@gziolo gziolo requested a review from pento Nov 8, 2018

* @since 4.3.0
*/
function gutenberg_register_packages_scripts() {
$packages_dependencies = array(

This comment has been minimized.

@gziolo

gziolo Nov 8, 2018

Member

Could we use JSON file instead?

This comment has been minimized.

@gziolo

gziolo Nov 16, 2018

Member

I opted for PHP file which returns an array instead.

@mcsf

This looks like a good execution.

'wp-deprecated',
'wp-escape-html',
'wp-polyfill',
),

This comment has been minimized.

@mcsf

mcsf Nov 13, 2018

Contributor

Noticing that 5.0's rich-text still includes wp-blocks as a dependency.

This comment has been minimized.

@gziolo

gziolo Nov 16, 2018

Member

Yes, and you can find wp-polyfill-ecmascript somewhere there, too. This PR tries to have an easy way to keep those 2 in sync.

@gziolo gziolo force-pushed the try/mirror-packages-config-core branch 2 times, most recently from fb123a6 to 69f5a7b Nov 16, 2018

@gziolo gziolo added this to the 4.5 milestone Nov 16, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 16, 2018

It all should be sorted out now. It's ready for review.

@gziolo gziolo requested review from youknowriad and omarreiss Nov 16, 2018

Show resolved Hide resolved lib/client-assets.php Outdated
* @package gutenberg
*/
return array(

This comment has been minimized.

@aduth

aduth Nov 16, 2018

Member

I'd not known you could return from a file in PHP to be used in a variable assignment. Pretty neat, though I could imagine the non-traditional nature of it could be viewed by some as a negative. Unsure there's any convention around it. I don't find it particularly offensive; it seems to serve a similar purpose to require( './foo.json' ) in Node. As purely static data, it could make sense as JSON, but I don't know that PHP provides any options for easily evaluating a JSON file to its equivalent array form. At least not without jumping through potential non-performant hoops involving file_get_contents and json_decode.

http://www.php.net/manual/en/function.return.php

This comment has been minimized.

@gziolo

gziolo Nov 16, 2018

Member

This is a very common technique in various PHP frameworks, where you optimize JSON or YAML config files by caching them as an array. I have no idea whether there are some patterns for that in core but I used that in the past many times.

Show resolved Hide resolved lib/client-assets.php Outdated
Show resolved Hide resolved webpack.config.js Outdated
Show resolved Hide resolved webpack.config.js Outdated
*/
function gutenberg_register_packages_scripts() {
$packages_dependencies = array(
'api-fetch' => array( 'wp-polyfill', 'wp-hooks', 'wp-i18n', 'wp-url' ),

This comment has been minimized.

@aduth

aduth Nov 16, 2018

Member

There's a bit of me which is sad in losing the explicitness, at least in considering package-dependencies.php as an exhaustive set. I suppose it is more an implicit thing if we're making a blanket statement that all new scripts should define wp-polyfill as a dependency.

@aduth

aduth approved these changes Nov 16, 2018

Core does a similar thing for vendor scripts. Is there a plan to shift the approach to align there as well?

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 16, 2018

Core does a similar thing for vendor scripts.

Oh, for sure. I didn't look at that part yet.

@gziolo gziolo force-pushed the try/mirror-packages-config-core branch from e925c55 to 86ca645 Nov 19, 2018

@gziolo gziolo merged commit 9622659 into master Nov 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the try/mirror-packages-config-core branch Nov 19, 2018

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 19, 2018

nice @gziolo 👍

@joemaller joemaller referenced this pull request Nov 21, 2018

Closed

RichText undefined #12173

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Config: Mirror packages dependencies registration with core (WordPres…
…s#11637)

* Config: Mirror packages dependencies registration with core

* Introduce package dependencies file

* Automate Webpack config by fetching list of packages from package.json file

* Make phpcs happy

* Sort packages dependencies alphabetically

* Update @SInCE comment for gutenberg_register_packages_scripts

* Address feedback from the review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment