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 back support for the assetPlugin option #118

Closed
wants to merge 1 commit into from

Conversation

ide
Copy link
Contributor

@ide ide commented Jan 11, 2018

Summary
Metro used to have support for "asset plugins", which allowed developers to specify arbitrary JS modules that could export a function for adding more fields to asset data objects. Some of this functionality was removed in the delta bundler work -- this PR adds it back.

Test plan
Made existing unit tests pass and added unit tests to test asset plugin behavior. Also tested E2E in a React Native project by adding assetPlugin=/path/to/pluginModule to a JS bundle URL and ensuring that the plugin ran.

Metro used to have support for "asset plugins", which allowed developers to specify arbitrary JS modules that could export a function for adding more fields to asset data objects. Some of this functionality was removed in the delta bundler work -- this PR adds it back.

Test Plan: Made existing unit tests pass and added unit tests to test asset plugin behavior. Also tested E2E in a React Native project by adding `assetPlugin=/path/to/pluginModule` to a JS bundle URL and ensuring that the plugin ran.
@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 Jan 11, 2018
@ide
Copy link
Contributor Author

ide commented Jan 11, 2018

create-react-native-app for RN 0.52 depends on this change, which is part of the motivation for this PR.

Copy link
Contributor

@rafeca rafeca left a comment

Choose a reason for hiding this comment

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

Thanks for adding this feature back to metro @ide !

I indeed removed it when refactoring the bundler

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.

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

facebook-github-bot pushed a commit that referenced this pull request Jan 12, 2018
Summary:
**Summary**
Metro used to have support for "asset plugins", which allowed developers to specify arbitrary JS modules that could export a function for adding more fields to asset data objects. Some of this functionality was removed in the delta bundler work -- this PR adds it back.

**Test plan**
Made existing unit tests pass and added unit tests to test asset plugin behavior. Also tested E2E in a React Native project by adding `assetPlugin=/path/to/pluginModule` to a JS bundle URL and ensuring that the plugin ran.
Closes #118

Differential Revision: D6711094

Pulled By: rafeca

fbshipit-source-id: f42c54cfd11bac5103194f85083084eef25fa3cd
@ide
Copy link
Contributor Author

ide commented Jan 12, 2018

Thanks for the review and updating the Jest config in RN too! Would it be possible to publish 0.24.4 to npm so that installing RN 0.52 (which depends on ^0.24.1) will get this update?

@rafeca
Copy link
Contributor

rafeca commented Jan 12, 2018

@ide I've just published v0.24.4 to npm, let me know if there is any issue

@ide ide deleted the asset-plugins branch January 12, 2018 18:12
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.

3 participants