Skip to content
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

Bundle some ui dependencies separately to limit the build size of ui.js #5547

Merged
merged 1 commit into from
Nov 7, 2018

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Oct 19, 2018

This PR has the following change on our builds:

  • production builds will now bundle some front end dependencies - namely, all react libraries, and the material-ui library - separately from ui.js
  • this causes the build size of ui.js to decrease by almost a megabyte

The smaller build size is needed at this time in order for us to pass the file size requirement of the mozilla linter, which we now run every build against with circle ci (e.g. https://circleci.com/gh/MetaMask/metamask-extension/64220?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link)

This was motivated by work I am doing on a feature branch which failed that step of the build.

The react and material-ui libraries here chosen for inclusion in a separate bundle were chosen because they:

  • are clearly front-end dependencies, and are likely to avoid any gulpfile mistakes due to confusion with backend dependencies
  • are mostly large in size, and therefore achieve the bundle size reduction goals
  • (at least in the case of the react based dependencies) form a coherent group and therefore a coherent bundle

Alternatively, we could build all front-end dependencies separately. That may, however, move us further from an ideal direction. The current PR meets an immediate need, as the file size failure of the mozilla linter is blocking current feature work. In the future, an more useful, generalizable and powerful approach would be to create separate bundles for separate parts of the app.

To help you understand the implementation. Browserify's .require() call is used to put a list of dependencies into its own bundle, and the .external() call is used to keep a dependency out of a bundle and instead ensure that the bundle accesses the dependency from an external file. See https://github.com/browserify/browserify#multiple-bundles

Finally, I'll note some improvements we can make in the future:

  • dynamically sourcing the list of dependencies or files to include in separate bundles
  • dynamically inserting script tags into the appropriate *.html files

@danjm danjm force-pushed the front-ends-deps-gulp-bundle branch from 97d9c32 to b262fb4 Compare October 19, 2018 03:06
@kumavis
Copy link
Member

kumavis commented Oct 21, 2018

@danjm have you investigated the test failures?

@danjm danjm force-pushed the front-ends-deps-gulp-bundle branch 4 times, most recently from b5b0e50 to e588f37 Compare October 23, 2018 00:44
@danjm danjm force-pushed the front-ends-deps-gulp-bundle branch 4 times, most recently from 111afe7 to 836ba3c Compare October 24, 2018 02:46
@danjm
Copy link
Contributor Author

danjm commented Oct 24, 2018

@kumavis I fixed all the test failures except for this one https://circleci.com/gh/MetaMask/metamask-extension/66399?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Which I've spent some time on but have not really been able to sort out. Have you seen that anywhere else?

@whymarrh
Copy link
Contributor

@danjm the drizzle builds seem to be passing on develop now (?) so you can try rebasing this

@danjm danjm force-pushed the front-ends-deps-gulp-bundle branch from 836ba3c to 43ca387 Compare October 31, 2018 17:05
@danjm danjm force-pushed the front-ends-deps-gulp-bundle branch from 43ca387 to da5f4a7 Compare October 31, 2018 23:17
@metamaskbot
Copy link
Collaborator

Builds ready [da5f4a7]: mascara, chrome, firefox, edge, opera

Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

This is great work @danjm!

@danjm
Copy link
Contributor Author

danjm commented Nov 1, 2018

@kumavis Is this change okay with you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants