Make WordPress Core

Opened 5 years ago

Last modified 4 years ago

#49423 accepted enhancement

Build: Create Webpack build configuration for vendor dependencies

Reported by: aduth's profile aduth Owned by: whyisjake's profile whyisjake
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch needs-refresh
Focuses: javascript Cc:

Description

Related: #49360, #46110 (could be considered a blocker for #49360)

The current process for including third-party JavaScript dependencies (jQuery, Moment, etc) occurs as a combination of:

For both of these, they are performed as simple file copying from node_modules.

This has a few downsides, related to depending on the package to prepackage "browser-ready" scripts:

  • Cannot use a package if it uses CommonJS or other module import semantics (See: ticket:49360#comment:6)
  • Cannot use a package if its code is not transpiled to run in ES5 environments like Internet Explorer (see #46110)
  • We must run Uglify minification directly for packages if it does not include a minified distributable

All of these problems could be addressed by incorporating the vendor dependencies into the Webpack build process.

  • Webpack can resolve module import semantics and output browser-ready artifacts
  • When configured to use the package's default entrypoint (package.json main) and assuming that entrypoint is ES5-ready, the output should also be ES5-ready. Note that the problems of #46110 are a bit more nuanced, related to upstream issues of the package in question. We probably want to expect that dependencies are already transpiled; ideally we would not need to incorporate something like Babel as part of this configuration.
  • Webpack has built-in minification handling.

This could technically be done within the existing packages configuration, but there's enough differences between packages and vendor scripts (both technically and conceptually) that it would make sense to extract this to its own configuration. There is already precedent here with having a separate configuration for media scripts.

See attached patch for proposed implementation.

Attachments (5)

49423-vendor-webpack.diff (6.4 KB) - added by aduth 5 years ago.
49423-vendor-webpack-2.diff (26.1 KB) - added by aduth 5 years ago.
49423-vendor-webpack-3.diff (26.0 KB) - added by bookdude13 5 years ago.
Refreshed patch for #49360
editor erros.PNG (47.5 KB) - added by bookdude13 5 years ago.
Errors on edit page
49423-latest.diff (6.0 KB) - added by gziolo 4 years ago.

Download all attachments as: .zip

Change History (32)

#1 @aduth
5 years ago

Note that the problems of #46110 are a bit more nuanced, related to upstream issues of the package in question. We probably want to expect that dependencies are already transpiled; ideally we would not need to incorporate something like Babel as part of this configuration.

To elaborate on this, specifically in regards to #46110.

I expect that 49423-vendor-webpack.diff would fix #46110.

This is because the entrypoint of formdata-polyfill is the minified copy:

https://github.com/jimmywarting/FormData/blob/e52a90ceb2e3424e5e2203760ecaeef21dbc5186/package.json#L5

The minified copy (but not the original source copy) is ES5-ready:

https://unpkg.com/browse/formdata-polyfill@3.0.19/formdata.min.js

Technically this means that both the .js and .min.js versions generated from the build would be minified. I don't see this as strictly problematic, in that (a) we're just using the default entrypoint and (b) while main should be unminified, this is a problem of the upstream package.

#2 @netweb
5 years ago

Sounds good, a quick glance I see uglify is removed from the patch, Uglify is still required for the natice JS files included in core to minify the files from the Grunt task here:

https://github.com/WordPress/wordpress-develop/blob/3dd39ffecea808afe86b8071331551d9d5e8cf89/Gruntfile.js#L687-L756

#3 follow-up: @aduth
5 years ago

@netweb Does that require the uglify-js package directly? I expect it should be satisfied by grunt-contrib-uglify:

https://github.com/WordPress/wordpress-develop/blob/3dd39ffe/package.json#L48

I'm able to run the task without error even after uninstalling uglify-js.

I noticed after removing the package that package-lock.json doesn't change much, indicating that there's other dependencies which still rely on it (I assume this one included).

Last edited 5 years ago by aduth (previous) (diff)

#4 in reply to: ↑ 3 @netweb
5 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 5.4

Replying to aduth:

@netweb Does that require the uglify-js package directly? I expect it should be satisfied by grunt-contrib-uglify:

Right you are, thanks, I tend to forget about those Grunt plugins in this new npm world ;)

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


5 years ago

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


5 years ago

#7 @bookdude13
5 years ago

  • Keywords needs-refresh added; commit removed

After building with this patch, I'm getting a 404 on wp-polyfill.js (url: http://localhost/wordpress-develop/src/wp-includes/js/dist/vendor/wp-polyfill.js?ver=7.4.4). Showing up in chrome and ff when on the root wp-admin page. (Edge not tested)

#8 @aduth
5 years ago

There was a small, careless issue in the original patch 49423-vendor-webpack.diff: The Webpack output was configured to wp-includes/js/dist, instead of the correct wp-includes/js/dist/vendor. This is corrected in 49423-vendor-webpack-2.diff .

#9 @bookdude13
5 years ago

  • Keywords needs-testing added; needs-refresh removed

Updating keywords so this can get tested

@bookdude13
5 years ago

Refreshed patch for #49360

@bookdude13
5 years ago

Errors on edit page

#10 @bookdude13
5 years ago

  • Keywords needs-refresh added; needs-testing removed

After the changes in #34960 this patch needed refreshing. It cleanly applies, but getting some include errors on the edit page and I don't have time now to figure out why.

#11 @bookdude13
5 years ago

  • Keywords needs-testing added; needs-refresh removed

Never mind, my build environment wasn't clean between testing stuff. Latest patch works.

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


5 years ago

#13 @SergeyBiryukov
5 years ago

  • Milestone changed from 5.4 to 5.5

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


4 years ago

#15 @davidbaumwald
4 years ago

  • Keywords needs-refresh added

Latest patch fails against trunk, so marking with needs-refresh.

#16 @whyisjake
4 years ago

  • Owner set to whyisjake
  • Status changed from new to accepted

#17 @gziolo
4 years ago

Do you need help to land this patch?

@gziolo
4 years ago

#18 @gziolo
4 years ago

I updated the patch to apply cleanly in the most recent version of trunk. Unfortunately, I see the same errors that @bookdude13 reported. It looks like those vendor packages don't get exposed as globals (lodash, moment, etc.).

Last edited 4 years ago by gziolo (previous) (diff)

#19 @gziolo
4 years ago

  • Milestone changed from 5.5 to 5.6

It won't be ready for 5.5. Moving to 5.6 Milestone.

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


4 years ago

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


4 years ago

#22 @johnbillion
4 years ago

  • Keywords needs-testing removed

@gziolo Hello! Is this ticket on your radar for 5.6 or shall we punt it?

#23 @whyisjake
4 years ago

  • Milestone changed from 5.6 to 5.7

#24 @gziolo
4 years ago

Nope, it didn't work with the patch proposed. I don't plan to work on it in the upcoming weeks. There is now webpack 5 available so I guess, it should be addressed first before this one.

Last edited 4 years ago by gziolo (previous) (diff)

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


4 years ago

#26 @noisysocks
4 years ago

  • Milestone changed from 5.7 to Future Release

Moving this to Future Release since it doesn't look like this will land in any immediate Core release.

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


4 years ago

Note: See TracTickets for help on using tickets.