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

Add custom serializer #309

Closed
wants to merge 3 commits into from
Closed

Conversation

raejin
Copy link
Contributor

@raejin raejin commented Nov 8, 2018

Summary
We would like to bundle an entryfile differently than it is currently done in Metro, specifically in development. Without a pluggable serializer, we wouldn't be able to perform bundle splitting in development, which is impactful to our development speed.

This PR alleviates bundle sizing problem by introducing a customSerializer instead of using the plainJSBundle.

Some minor question, do I have to implement this in the build public method in the server?

Test plan

  • Wrote a customSerializer and test it against the new change in metro. Our bundle was able to do what we needed it to accomplish.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 8, 2018
@codecov-io
Copy link

codecov-io commented Nov 8, 2018

Codecov Report

Merging #309 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #309      +/-   ##
=========================================
- Coverage   86.31%   86.3%   -0.01%     
=========================================
  Files         163     163              
  Lines        4916    4908       -8     
  Branches      753     752       -1     
=========================================
- Hits         4243    4236       -7     
+ Misses        600     599       -1     
  Partials       73      73
Impacted Files Coverage Δ
packages/metro-config/src/defaults/index.js 85% <ø> (ø) ⬆️
...etro/src/DeltaBundler/Serializers/plainJSBundle.js 100% <ø> (ø) ⬆️
packages/metro-config/src/convertConfig.js 100% <ø> (ø) ⬆️
packages/metro/src/Server.js 86.91% <100%> (+0.11%) ⬆️
...metro/src/ModuleGraph/output/indexed-ram-bundle.js 100% <0%> (ø) ⬆️
...rc/ModuleGraph/output/multiple-files-ram-bundle.js 100% <0%> (ø) ⬆️
packages/metro/src/ModuleGraph/output/util.js 93.47% <0%> (+0.62%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 568ec61...fd11060. Read the comment docs.

packages/metro/src/Server.js Outdated Show resolved Hide resolved
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@mjesun has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

thymikee added a commit to thymikee/metro that referenced this pull request Mar 3, 2019
…ation

* upstream/master: (42 commits)
  Bump metro@0.49.1
  Fix entryFile always is assumed to end with `.js` extension (facebook#310)
  Adds support for `publicPath` to enable serving assets from different locations. (facebook#299)
  Make dynamic import interoperable for CJS and ESM
  Add custom serializer (facebook#309)
  React sync for revisions 4773fdf...3ff2c7c
  metro-buck: disable inline IDs in dev mode
  Simplify helper function
  Rewrite traverseDependency-integration-tests to make them module resolution unit tests
  RN: Revert React 16.6 Sync
  React sync for revisions 4773fdf...bf9fadf
  Bump react-deep-force-update
  Use getFileIterator() from jest-haste-map
  Deploy Flow v0.85 to xplat/js
  metro-buck: add explicit import() prefetch feature
  jest: remove jasmine usages
  jest: upgrade to 24.0.0-alpha.4
  Fix source map loading in the remote debugger
  Fix metro visualizer transformation
  Update babel to version 7
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants