Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#50460 new enhancement

Don't minimize the `script-loader-packages.php` file

Reported by: desrosj's profile desrosj Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-patch close
Focuses: Cc:

Description

The wp-includes/assets/script-loader-packages.php file was added in #48154 through [47352] as a means to automate the process of updating the NPM packages from the Gutenberg repository (including dependencies and versions) in a way that the packages would still be available through the wp_(enqueue|register)_scripts().

This works great, but the file is a single line return statement that returns a multidimensional array, which makes it impossible to read through without reformatting it.

I am proposing that the file not be minimized to one line in order to make it human readable. This would have an added benefit of making the changesets readable, allowing someone to view a changeset and understand exactly what was changed or updated (see [47920], [47517], [47513]).

Change History (13)

#1 @desrosj
4 years ago

The generation of this file is handled in Webpack through the Dependency Extraction plugin in the Gutenberg repo. It doesn't seem that the output can be formatted in anything but single line looking over how that code works.

It may be easier to handle this in Core after the file is created.

#2 @johnbillion
3 years ago

There's a PHP plugin for Prettier, so the output could be post-processed with that.

https://github.com/prettier/plugin-php

This ticket was mentioned in Slack in #core by desrosj. View the logs.


3 years ago

#4 @desrosj
3 years ago

  • Milestone changed from Awaiting Review to 5.8

I'm going to try and tackle this for 5.8. This is a huge pain when comparing updates to this file.

#5 @johnjamesjacoby
3 years ago

  • Milestone changed from 5.8 to Awaiting Review

I am +1 to not minimizing this file.

It is helpful – at least to my human eyes – to visually verify the changes that get auto-generated into it.

#6 @peterwilsoncc
3 years ago

My preference is to stop committing it (see #49635) as it's ignored on the build server which uses the npm generated version. @azaozz had some minor concerns about doing so but suggested biting the bullet, my inclination is to do so for 5.8.

#7 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.8

#8 @desrosj
3 years ago

  • Keywords close added

Ah, thanks for pointing #49635 out, @peterwilsoncc. I had missed that one. I think that eliminating the need to include this file is preferable.

I'll leave this open for now, but only need to tackle this if the fix suggested in #49635 is not committed.

#9 @gziolo
3 years ago

Maybe we can tweak it in the webpack plugin, it looks like the following lines are responsible of the output format:

https://github.com/WordPress/gutenberg/blob/48e470eca70966c8a9ff89f342c875b7aa9845b0/packages/dependency-extraction-webpack-plugin/lib/index.js#L95-L99

#10 @gziolo
3 years ago

The npm library we use to output PHP code from the JSON representation can be always inlined and modified to meet our requirements:
https://github.com/daniel-zahariev/json2php/blob/master/lib/json2php.js

Unless we find a way to run unit tests without wp-includes/assets/script-loader-packages.php file present as discussed in #49635.

This ticket was mentioned in Slack in #core by lukecarbis. View the logs.


3 years ago

#12 @desrosj
3 years ago

  • Milestone changed from 5.8 to 5.9

I'm going to punt this and #49635 to 5.9 as they're not quite ready, and today is feature freeze.

Test and build tool changes are allowed after feature freeze up until RC, so if someone is able to work on this before then, it can always be moved back.

#13 @hellofromTonya
3 years ago

  • Milestone changed from 5.9 to Future Release

Well boo, it didn't get attention during the 5.9 cycle either. Today is Feature Freeze.

Like before, I'll punt this and #49635 to Future Release (as 6.0 isn't available yet for selection).

As @desrosj noted last time, test and build tool changes are allowed after feature freeze up until RC, so if someone is able to work on this before then, it can always be moved back.

Note: See TracTickets for help on using tickets.