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

Strip @providesModule directives #84

Closed
wants to merge 1 commit into from
Closed

Conversation

zpao
Copy link
Member

@zpao zpao commented Nov 24, 2015

This should save us from some future headaches with situations like RN where @providesModule copies of some of these exist.

sophiebits added a commit to facebook/react-native that referenced this pull request Dec 30, 2015
Summary:
We don't (yet) treat these the same as any other modules because we still have special resolution rules for them in the packager allowing the use of `providesModule`, but I believe this allows people to use npm react in their RN projects and not have duplicate copies of React. Fixes #2985.

This relies on fbjs 0.6, which includes `.flow` files alongside the `.js` files to allow them to be typechecked without additional configuration. This also uses react 0.14.5, which shims a couple of files (as `.native.js`) to avoid DOM-specific bits. Once we fix these in React, we will use the same code on web and native. Hopefully we can also remove the packager support I'm adding here for `.native.js`.

This diff is not the desired end state for us – ideally the packager would know nothing of react or fbjs, and we'll get there eventually by not relying on `providesModule` in order to load react and fbjs modules. (fbjs change posted here but not merged yet: facebook/fbjs#84.)

This should also allow relay to work seamlessly with RN, but I haven't verified this.

public

Reviewed By: sebmarkbage

Differential Revision: D2786197

fb-gh-sync-id: ff50f28445e949edc9501f4b599df7970813870d
@sophiebits
Copy link
Contributor

Happened in #95.

@sophiebits sophiebits closed this Jan 5, 2016
@skevy
Copy link
Contributor

skevy commented Jan 6, 2016

Question to @zpao and @spicyj - should we also be stripping @providesModule from the .flow.js files? Or does it not matter?

@sophiebits
Copy link
Contributor

Yes, probably.

@zpao zpao deleted the strip-pm branch February 10, 2016 22:32
cpojer pushed a commit to facebook/metro that referenced this pull request Jan 26, 2017
Summary:
We don't (yet) treat these the same as any other modules because we still have special resolution rules for them in the packager allowing the use of `providesModule`, but I believe this allows people to use npm react in their RN projects and not have duplicate copies of React. Fixes facebook/react-native#2985.

This relies on fbjs 0.6, which includes `.flow` files alongside the `.js` files to allow them to be typechecked without additional configuration. This also uses react 0.14.5, which shims a couple of files (as `.native.js`) to avoid DOM-specific bits. Once we fix these in React, we will use the same code on web and native. Hopefully we can also remove the packager support I'm adding here for `.native.js`.

This diff is not the desired end state for us – ideally the packager would know nothing of react or fbjs, and we'll get there eventually by not relying on `providesModule` in order to load react and fbjs modules. (fbjs change posted here but not merged yet: facebook/fbjs#84.)

This should also allow relay to work seamlessly with RN, but I haven't verified this.

public

Reviewed By: sebmarkbage

Differential Revision: D2786197

fb-gh-sync-id: ff50f28445e949edc9501f4b599df7970813870d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants