Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Remove vendors.js as a frontend dependency #750

Merged
merged 6 commits into from Jul 22, 2019

Conversation

Aljullu
Copy link
Contributor

@Aljullu Aljullu commented Jul 18, 2019

Depends on #746.

This PR:

  • Removes vendors.js as a frontend dependency.
  • Uses externals for dependencies, so they are shared between plugins and blocks.
  • Merges packages.js and vendors.js files. They were split so vendors.js could be included in the frontend, but that is no longer necessary.

How to test the changes in this Pull Request:

Test vendors.js is no longer a dependency:

  1. Create a post and add a Product Categories List block.
  2. Open the post in the frontend and verify it works as expected.
  3. In the Network tab of your browser devtools, verify no vendors.js file was loaded.

Test we are using externals

  1. Install Jetpack.
  2. Create a post and add a Product Categories List block, a Slideshow block and a Map block.
  3. Open the post in the frontend and, in the Network tab of your browser devtools, verify only one lodash.js file was loaded.

Changelog

Use externals for frontend dependencies so they are shared between extensions and blocks. That saves 2.57MB on page weight.

@Aljullu Aljullu requested a review from a team July 18, 2019 15:14
@Aljullu Aljullu self-assigned this Jul 18, 2019
@Aljullu Aljullu added this to In Progress PRs (for automation purposes) in Isotope via automation Jul 18, 2019
@Aljullu Aljullu added this to the 2.3 milestone Jul 18, 2019
Copy link
Contributor

@psealock psealock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uses externals for dependencies, so they are shared between plugins and blocks.

Looking at the vendors.js compiled code, I still see things like lodash dependency. Only 1 request for lodash.js is made, but the dependency is also shipped in vendors.

externals: [ lodash: 'lodash' ], in the webpack config should remove it from vendors and build the blocks js to look for lodash on the window.

EDIT: on frontend.js compiled code, I do see that lodash is not included:

/***/ "lodash":
/*!**********************************!*\
  !*** external {"this":"lodash"} ***!
  \**********************************/

@Aljullu
Copy link
Contributor Author

Aljullu commented Jul 19, 2019

@psealock thanks for taking a look!

EDIT: on frontend.js compiled code, I do see that lodash is not included:

Right, this PR only refers to the frontend. I created a PR to fix this in wp-admin (#753). Do you think we should prioritize that too or we can ship it in the next version (2.4)?

@Aljullu Aljullu requested a review from psealock July 19, 2019 15:00
@psealock
Copy link
Contributor

Do you think we should prioritize that too or we can ship it in the next version (2.4)?

I think its an easy win with a few changes to the webpack config (I hope its easy at least), so may as well try to get it in.

Copy link
Contributor

@psealock psealock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Nice improvement here. Do you have a before/after file size? It might be nice to include in the release notes.

@Aljullu
Copy link
Contributor Author

Aljullu commented Jul 22, 2019

I think its an easy win with a few changes to the webpack config (I hope its easy at least), so may as well try to get it in.

👍

Looking good! Nice improvement here. Do you have a before/after file size? It might be nice to include in the release notes.

Basically it saves 2.57MB of vendors.js because externals were included anyway (even though they were not being used). I added the number to the changelog entry.

@Aljullu Aljullu changed the base branch from fix/load-frontend-scripts-only-when-required to release/2.3 July 22, 2019 09:37
@Aljullu Aljullu merged commit ba7cdcb into release/2.3 Jul 22, 2019
Isotope automation moved this from In Progress PRs (for automation purposes) to Done Sprint 21 (July 9 - July 23) Jul 22, 2019
@Aljullu Aljullu deleted the fix/remove-vendors-dependency branch July 22, 2019 09:39
@Aljullu Aljullu added the release: cherry-pick Cherry picked into the relevant release branch. label Jul 22, 2019
Aljullu added a commit that referenced this pull request Jul 22, 2019
* Load frontend scripts only when required

* Typo

* Use register_script from Assets

* Remove vendors as a frontend dependency

* Simplify webpack config
@Aljullu Aljullu removed the release: cherry-pick Cherry picked into the relevant release branch. label Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants