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

Jump to bottom

Add transformOptions to each graph. Update tests. #679

Closed
wants to merge 4 commits into from
Closed

Add transformOptions to each graph. Update tests. #679

wants to merge 4 commits into from

Conversation

afoxman
Copy link
Contributor

@afoxman afoxman commented Jun 16, 2021

Summary

Each graph is built using a specific set of transformOptions. Bind these to the graph, making them available to extensibility APIs such as customSerializer and experimentalSerializerHook. This data -- specifically, platform -- makes it possible to perform TypeScript validation during bundle/serve scenarios. platform is used to resolve types for platform-specific files.

Test plan

I updated existing Metro tests to validate the transformOptions in a graph.

I also wrote a sample application which bundles using Metro, and uses experimentalSerializerHook. I had my app print platform to the console, verifying that it was the correct value.

@facebook-github-bot
Copy link
Contributor

Hi @afoxman!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@afoxman
Copy link
Contributor Author

afoxman commented Jun 16, 2021

@motiz88

@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 Jun 16, 2021
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

acoates-ms
acoates-ms approved these changes Jun 17, 2021
View reviewed changes
packages/metro/src/DeltaBundler.js Outdated Show resolved Hide resolved
@motiz88
Copy link
Contributor

motiz88 commented Jun 17, 2021

Instead of explicitly threading transformOptions as a new argument through getDependencies, buildGraph, DeltaCalculator, etc - can we add it as a property to the Options type? Seems like we already pass that around to most of the call sites that care about transformOptions, and it makes sense semantically IMO. That would make for a smaller change that I'd be a bit more comfortable maintaining, especially since we don't have a test that captures what all this passing-transformOptions-around is for.

On that note, it's not ideal that we don't have an existing test for experimentalSerializerHook for you to update here. If we go with the less-invasive approach above, I think we can live with that for now (given that it's an experimental feature, and that this isn't a breaking change). But definitely feel free to write a test if you want to :)

@afoxman
Copy link
Contributor Author

afoxman commented Jun 18, 2021 •
edited

Plumbing work is done. I took a look at adding tests, but I couldn't find a place to easily add them. Looks like we'd need a new test suite under /metro/src/tests/IncrementalBundler-test.js, which means it would need to cover the full IncrementalBundler class. I'm not that much of a Metro expert yet. :)

If you have advice on another place I can add a few experimentalSerializerHook tests, let me know.

motiz88
motiz88 approved these changes Jun 23, 2021
View reviewed changes
Copy link
Contributor

@motiz88 motiz88 left a comment

Choose a reason for hiding this comment

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

lgtm with one minor comment.

packages/metro/src/DeltaBundler/types.flow.js Show resolved Hide resolved
@afoxman @motiz88
Update packages/metro/src/DeltaBundler/types.flow.js
83d58f9 
Co-authored-by: Moti Zilberman <motiz88@gmail.com>
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@motiz88 merged this pull request in 57106d2.

@afoxman afoxman deleted the serializer-hook branch June 24, 2021 20:37
facebook-github-bot pushed a commit that referenced this pull request Jul 1, 2021
@motiz88 @facebook-github-bot
Publish 0.66.1
2b44ec3 
Summary:
Draft changelog:

* **[Feature]** Add option to use the Hermes parser in the transformer. (6dae163)
* **[Feature]** Update the list of default React Native lazy imports to match the [removal of Picker](facebook/react-native@ad0ccac), which was already deprecated. (7dcb20d)
* **[Feature]** Make the input transform options (e.g. `platform`) available to `experimentalSerializerHook` and `customSerializer`. (#679)
* **[Fix]** Avoid triggering Babel scope caching bugs. (4fea2bd, 1370b7a)
* **[Fix]** Fix Hermes debugger proxy for IPv6. (#662)
* **[Types]** Remove documentation and types for the `postMinifyProcess` option which has been broken since Metro v0.33.0. (5d778de)
* **[Experimental]** Various unstable additions to the transformer config. *These are not covered by semver and can change at any time.* (6482241, 3390274, 5b913fa, #677)

Reviewed By: GijsWeterings

Differential Revision: D29514800

fbshipit-source-id: 89bd4b8a53a701ceaf18867e8e9379fd3401bbbe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@motiz88 motiz88 motiz88 approved these changes

@acoates-ms acoates-ms acoates-ms approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@afoxman @facebook-github-bot @motiz88 @acoates-ms