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

[react-native] Default to lazy requires for all public RN exports #362

Closed
wants to merge 2 commits into from

Conversation

ide
Copy link
Contributor

@ide ide commented Feb 22, 2019

Summary

This commit changes metro-react-native-babel-preset to emit lazy require calls for all of RN's public exports. This is so that we can move RN to use ES import/export, which syntactically have no lazy version, while preserving the lazy behavior we have today by default.

If the developer specifies lazyImportExportTransform: true, all imports/exports will be lazy including RN's public exports, so they will remain lazy. (Also, this PR allows developers to specify non-boolean values for lazyImportExportTransform e.g. an array of whitelisted paths.)

Note: this commit must land and be published before RN can use ESM since the iOS E2E tests depend on the current lazy require calls.

See the draft PR for RN here: facebook/react-native#23591

Test plan

Used this module inside of the RN E2E iOS tests and loaded the bundle successfully. Also used it in RNTester.

This commit changes `metro-react-native-babel-preset` to emit lazy `require` calls for all of RN's public exports. This is so that we can move RN to use ES import/export, which syntactically have no lazy version, while preserving the lazy behavior we have today by default.

If the developer specifies `lazyImportExportTransform: true`, all imports/exports will be lazy including RN's public exports, so they will remain lazy.

Note: this commit must land and be published before RN can use ESM since the iOS E2E tests depend on the current lazy `require` calls.

Test Plan

Used this module inside of the RN E2E iOS tests and loaded the bundle successfully. Also used it in RNTester.
@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 Feb 22, 2019
@codecov-io
Copy link

codecov-io commented Feb 22, 2019

Codecov Report

Merging #362 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #362      +/-   ##
==========================================
+ Coverage      85%   85.01%   +<.01%     
==========================================
  Files         170      171       +1     
  Lines        5234     5237       +3     
  Branches      800      800              
==========================================
+ Hits         4449     4452       +3     
  Misses        698      698              
  Partials       87       87
Impacted Files Coverage Δ
...ct-native-babel-preset/src/configs/lazy-imports.js 100% <100%> (ø)
...etro-react-native-babel-preset/src/configs/main.js 100% <100%> (ø) ⬆️

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 6fe1592...94ec088. Read the comment docs.

@cpojer
Copy link
Contributor

cpojer commented Feb 25, 2019

cc @mjesun

We discussed this for RN's react-native-implementation, does the change to support this in Metro make sense to you?

@mjesun
Copy link
Contributor

mjesun commented Feb 25, 2019

Yeah, this makes sense to me; although my suggestion is to lazify everything :)

Metro has anyway native support for import/export, but this was mentioned for tests, where you’ll still need the Babel transform; so happy to get that in.

@ide
Copy link
Contributor Author

ide commented Mar 7, 2019

Would be nice to move towards making more things lazy (ex: if package.json contains a "lazyImport" flag). In the short term, it'd be nice to have this RN-specific change for RN 0.60. On Expo's side we can patch stuff but for people not using Expo it probably makes sense to land the same performance improvements (lazy imports -> opens the door for moving to import/export -> enables dead-export elimination).

@matthargett
Copy link

Right now we have a Babel transform that does this conversion so tree shaking (and smaller bundle size) is possible. It would be great to be able to remove that transform for faster bundle times with RN 0.60.

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

I removed takeSnapshot from the list because it is imminently being removed, and we have to find a way to keep this list up-to-date, but it looks good otherwise. Let's ship it.

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.

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ide
Copy link
Contributor Author

ide commented Mar 13, 2019

we have to find a way to keep this list up-to-date

Broadly speaking I agree it'd be better to update the list periodically but things will generally work even if the list gets out of date so I'm not so worried. If exports are removed from RN (ex: lean core) then those entries on this list will be no-ops, and if new exports are added to RN then they won't get transformed into lazy requires. Neither is ideal but also neither is a severe build breaker.

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

* upstream/master:
  Upgrade jest to 24.5.0
  Bump Prettier to 1.16.4
  Bump Metro to v0.53.1
  Another pass at Metro Flow annotation coverage
  Add CLI option to ignore function map
  Default to lazy requires for all public RN exports (facebook#362)
  FileStore now recreates parent cache folders if a write fails (facebook#370)
  Support symbolicating file:line:column stack frames
  Add connection verification
  Improve Flow annotation coverage in Metro
  Drop leading newline
  Deploy 0.94 to xplat
  Add new metro flag for running inspector proxy
  Perform security fixes
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.

6 participants