[RFC] Remove knowledge of fbjs from the packager by skevy · Pull Request #5084 · react/react-native

skevy · 2016-01-03T01:50:12Z

As @spicyj mentioned in commit 6a838a4, the ideal state of affairs when it comes to consuming react and fbjs from NPM is for the packager not to have knowledge of either package. This PR addresses the fbjs part of that, and relies on facebook/fbjs#95. DO NOT MERGE until #95 (or a variation) is in fbjs and is released to npm.

This PR does several things:

Adds stub modules within RN that expose fbjs modules to be required using Haste. After discussing a few ideas with @spicyj, this seemed like a good option to keep internal FB devs happy (and not make them change the way they write JS), but allow for removing packager complexity and fit in better with the NPM ecosystem. Note -- it skips stubbing fetch, ExecutionEnvironment, and ErrorUtils, due to the fact that these need to have Native specific implementations, and there's no reason for those implementations to exist in fbjs.
Removes the modules that were previously being used in lieu of their fbjs equivalent modules, but didn't really need to be. These include: Map, isEmpty, crc32, and Promise. Promise is a special case - it has been moved to Promise.native.js in fbjs, so that a package such as Relay can require('fbjs/lib/Promise'), and not cause problems.
Removes the fbjs modules from the packager blacklist (yay!), and removes fbjs from the list of packages that the RN packager consumes Haste modules from. I left in the downstream modules, but I think those should really live within an FB-specific blacklist that you keep in your internal codebase. Let me know your thoughts there.
Adds fbjs to unmockedModulePathPatterns for Jest, to allow tests to not behave weirdly. This matches FB's internal Jest config.
Remove everything except fetch, ExecutionEnvironment, and ErrorUtils from the flowconfig ignore list.
Fix InteractionManager-test to make sure ErrorUtils is mocked. Quite honestly, I'm not sure why this was working before.

This, combined with facebook/fbjs#95 and a yet to be submitted Relay PR, allow for Relay to be used OOTB with React Native. Woo!

Looking forward to your comments :)

/cc @vjeux @spicyj

P.S. - The tests are going to fail here, until the fbjs dep is updated.

facebook-github-bot · 2016-01-03T01:50:33Z

By analyzing the blame information on this pull request, we identified @spicyj, @cpojer and @none to be potential reviewers.

vjeux · 2016-01-03T01:57:14Z

@martinbigio

vjeux · 2016-01-03T02:17:23Z

It's sad that we include a module named CSSCore in rn :(

We may not need to - I'd be happy to prune. But, I did see that CSSCore was blacklisted from downstream...so I thought there might be a place where you guys are using it internally...which is the only reason these stubs really exist anyway (otherwise we'd require from fbjs everywhere). But, I could be wrong. :)

Yeah, we have one upstream module requires CSSCore :(

martinbigio · 2016-01-03T18:34:48Z

I LOVE this change! wish one day we could remove providesModuleNodeModules support from the packager. We need to run both the oss and internal tests once fbjs#95 lands as many things could break. I don't have much to say from a review point of view, the change is pretty clean.

martinbigio · 2016-01-03T18:43:11Z

What if we have have stubs only internally on Facebook. Otherwise, people on OSS may get confused trying to figure out why we expose both CommonJS and Haste modules for fbjs.

skevy · 2016-01-03T19:11:16Z

@martinbigio the reason I added the stubs here was after talking to @spicyj. We discussed that it would be a good thing to keep RN code using haste, for consistency throughout the RN codebase. Otherwise...you have require('fbjs/lib/keyMirror') next to things like require('TouchEventUtils'). Right now, just about every require in the RN code base (save these new fbjs stubs) uses Haste. This consistency could be good if one day we decided to add a prepublish task that rewrites requires to CommonJS for use in somebody's Webpack build :)

skevy · 2016-01-03T19:12:40Z

@martinbigio regarding your comment about the tests...for what it's worth, I've run flow and all the tests locally with the fbjs change included (that wasn't an easy task haha)...and the changes required are included in this diff (skevy@7c00d73, skevy@42af07e, skevy@bc4486a).

martinbigio · 2016-01-04T00:05:26Z

@skevy sounds good. I'll import and test this internally once fbjs#95 lands

skevy · 2016-01-04T00:51:26Z

🚀

skevy · 2016-01-05T01:49:51Z

@martinbigio, just FYI, facebook/fbjs#95 is merged. :-)

martinbigio · 2016-01-05T16:53:28Z

@facebook-github-bot import

facebook-github-bot · 2016-01-05T16:54:22Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/461611524028597/int_phab to review.

davidaurelio · 2016-01-05T16:58:05Z

We might want to keep Promise.js, in order to be able to control warning behavior (for unhandled rejections)

skevy · 2016-01-05T17:26:44Z

@davidaurelio if we keep Promise, then projects like Relay that import "fbjs/lib/Promise" will break...hence me moving it to fbjs at Promise.native.js. Not sure if you have any thoughts on how that might be able to be fixed.

davidaurelio · 2016-01-06T09:51:46Z

I will work around it in YellowBox, then. I tried to land the upgrade to promise@7.1.1 yesterday, but I can’t resolve an internal issue :-(

skevy · 2016-01-07T23:21:35Z

@davidaurelio I saw you updated Promise.js -- I guess I should bring these changes over to fbjs's Promise.native.js?

skevy · 2016-01-07T23:34:15Z

@davidaurelio PR'ed facebook/fbjs#100

facebook-github-bot · 2016-01-07T23:42:14Z

@skevy updated the pull request.

davidaurelio · 2016-01-08T00:05:42Z

@skevy very cool, thank you! I will probably replace this with a configurable hook in the future, so that we don’t leak too much RN specific stuff into fbjs. That should also make stack trace formatting easier.

skevy · 2016-01-08T00:15:48Z

@davidaurelio that's totally fine, just in lieu of this getting merged (because it involves so many coordinating PRs and infra changes)...I wanted to make sure we were keeping up to date.

Summary: Turns out, even after discussion that was had in #5294 (comment), we really do need this transform. I've just included it in the preset...let me know if you all would rather publish to npm. The actual reason why this is necessary is because in the latest sync from FB, fbjs was updated to use the `Symbol.iterator` express in it's isEmpty function: facebook/fbjs@064a484 We use this in RN in the ListView...and this change (once #5084 is merged) will cause ListView to break on older JSC context's. This resolves that, and is probably something we should have had all along. Closes#5824 Reviewed By: svcscm Differential Revision: D2913315 Pulled By: vjeux fb-gh-sync-id: abaf484a9431b3111e8118d01db8d2c0d2dd73ca shipit-source-id: abaf484a9431b3111e8118d01db8d2c0d2dd73ca

Summary: This PR moves `react` from dependencies to peerDependencies. In general, this would have only been important for those people using packages that depend on `react` and were using npm@2...npm@3 would automatically de-dupe. However, when #5812 gets merged, dependencies will be scoped to react-native (on both npm@2 & npm@3), thus breaking projects that are using a package like `react-redux` for example, which depends on `react`. There would be two copies of React installed, and due to the use of haste modules in `react`, this would break the packager and cause naming collisions. This PR does three things - 1. Moves the dependency from dependencies to peerDependencies 2. Updates the local-cli to run `npm install react --save` when a new project is initialized. 3. Updates `react-native upgrade` to warn if `react` is not listed in the package.json's dependencies. **Note: This will require a shrinkwrap update.** Closes#5813 Reviewed By: svcscm Differential Revision: D2918380 Pulled By: androidtrunkagent fb-gh-sync-id: 6e4234a45284be2fdf6fedf29e70b2d2d0262486 shipit-source-id: 6e4234a45284be2fdf6fedf29e70b2d2d0262486

davidaurelio · 2016-02-23T21:42:14Z

Good news. Almost everything is green now. This will be coming soon

marcshilling · 2016-03-01T21:17:05Z

So this didn't make it into 0.21?

bestander · 2016-03-01T22:19:57Z

Yeah, that is a can of worms.
Still in progress

On Tuesday, 1 March 2016, Marc Shilling notifications@github.com wrote:

So this didn't make it into 0.21?
—
Reply to this email directly or view it on GitHub
#5084 (comment)
.

chirag04 · 2016-03-01T22:37:23Z

Using this in our fork and makes working with relay flawless. hope it gets merged soon.

davidaurelio · 2016-03-02T10:55:27Z

I’m through with all internal failures. CI is running right now, and I’m waiting for review. It’s coming.

Summary:Follow-up to react/react-native#5084 This… - changes all requires within RN to `require('fbjs/lib/…')` - updates `.flowconfig` - updates `packager/blacklist.js` - adapts tests - removes things from `Libraries/vendor/{core,emitter}` that are also in fbjs - removes knowledge of `fbjs` from the packager Closesreact/react-native#5084 public Reviewed By: bestander Differential Revision: D2926835 fb-gh-sync-id: 2095e22b2f38e032599d1f2601722b3560e8b6e9 shipit-source-id: 2095e22b2f38e032599d1f2601722b3560e8b6e9

ide · 2016-03-03T19:46:25Z

@skevy @davidaurelio you guys are heroes! thanks :D

(& fingers crossed we can land this in 0.22)

Kureev · 2016-03-03T21:23:14Z

thanks, guys ❤️

chirag04 · 2016-03-03T23:22:50Z

You guys are amazing. Thanks for all the work 👍

Summary:Follow-up to react/react-native#5084 This… - changes all requires within RN to `require('fbjs/lib/…')` - updates `.flowconfig` - updates `packager/blacklist.js` - adapts tests - removes things from `Libraries/vendor/{core,emitter}` that are also in fbjs - removes knowledge of `fbjs` from the packager Closesreact/react-native#5084 public Reviewed By: bestander Differential Revision: D2926835 fb-gh-sync-id: 2095e22b2f38e032599d1f2601722b3560e8b6e9 shipit-source-id: 2095e22b2f38e032599d1f2601722b3560e8b6e9

Summary: As spicyj mentioned in commit 6a838a4, the ideal state of affairs when it comes to consuming `react` and `fbjs` from NPM is for the packager not to have knowledge of either package. This PR addresses the `fbjs` part of that, and relies on facebook/fbjs#95. **DO NOT MERGE** until #95 (or a variation) is in `fbjs` and is released to npm. This PR does several things: 1. Adds stub modules within RN that expose `fbjs` modules to be required using Haste. After discussing a few ideas with spicyj, this seemed like a good option to keep internal FB devs happy (and not make them change the way they write JS), but allow for removing packager complexity and fit in better with the NPM ecosystem. Note -- it skips stubbing `fetch`, `ExecutionEnvironment`, and `ErrorUtils`, due to the fact that these need to have Native specific implementations, and there's no reason for those implementations to exist in `fbjs`. 2. Removes the modules that were previously being used in lieu of their `fbjs` eq Closesreact/react-native#5084 Reviewed By: bestander Differential Revision: D2803288 Pulled By: davidaurelio fb-gh-sync-id: fd257958ee2f8696eebe9048c1e7628c168bf4a2 shipit-source-id: fd257958ee2f8696eebe9048c1e7628c168bf4a2

Summary: As spicyj mentioned in commit 6a838a4, the ideal state of affairs when it comes to consuming `react` and `fbjs` from NPM is for the packager not to have knowledge of either package. This PR addresses the `fbjs` part of that, and relies on facebook/fbjs#95. **DO NOT MERGE** until #95 (or a variation) is in `fbjs` and is released to npm. This PR does several things: 1. Adds stub modules within RN that expose `fbjs` modules to be required using Haste. After discussing a few ideas with spicyj, this seemed like a good option to keep internal FB devs happy (and not make them change the way they write JS), but allow for removing packager complexity and fit in better with the NPM ecosystem. Note -- it skips stubbing `fetch`, `ExecutionEnvironment`, and `ErrorUtils`, due to the fact that these need to have Native specific implementations, and there's no reason for those implementations to exist in `fbjs`. 2. Removes the modules that were previously being used in lieu of their `fbjs` eq Closesreact/react-native#5084 Reviewed By: bestander Differential Revision: D2803288 Pulled By: javache fb-gh-sync-id: 121ae811ce4cc30e6ea79246f85a1e4f65648ce1 shipit-source-id: 121ae811ce4cc30e6ea79246f85a1e4f65648ce1

Summary:Follow-up to react/react-native#5084 This… - changes all requires within RN to `require('fbjs/lib/…')` - updates `.flowconfig` - updates `packager/blacklist.js` - adapts tests - removes things from `Libraries/vendor/{core,emitter}` that are also in fbjs - removes knowledge of `fbjs` from the packager Closesreact/react-native#5084 Reviewed By: bestander Differential Revision: D2926835 fb-gh-sync-id: 2095e22b2f38e032599d1f2601722b3560e8b6e9 shipit-source-id: 2095e22b2f38e032599d1f2601722b3560e8b6e9

skevy self-assigned this Jan 3, 2016

skevy added the Size/L label Jan 3, 2016

skevy added the Cleanup label Jan 3, 2016

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 3, 2016

skevy mentioned this pull request Jan 3, 2016
[RFC] RN 0.18 Compatibility changes facebook/fbjs#95
Merged

skevy mentioned this pull request Jan 3, 2016
React Native Compatibility facebook/relay#713
Closed

vjeux reviewed Jan 3, 2016
View reviewed changes

skevy mentioned this pull request Jan 3, 2016
Make Relay work with React Native out of the box facebook/relay#26
Closed
5 tasks

skevy mentioned this pull request Jan 4, 2016
API consistency in React Native #4971
Closed

skevy force-pushed the blacklist-cleanup branch from bc4486a to 5255659Compare January 7, 2016 23:41

skevy mentioned this pull request Feb 17, 2016
Make providesModuleNodeModules behavior to be more deterministic. #5985
Closed

skevy mentioned this pull request Feb 18, 2016
Fix providesModuleNodeModules behavior to be more deterministic. facebookarchive/node-haste#33
Merged

mkonicek mentioned this pull request Feb 19, 2016
Merge PRs that get master back to good state #6037
Closed

chirag04 added a commit to 4Catalyzer/react-native that referenced this pull request Feb 23, 2016
mergereact#5084
d63d697

jsierles mentioned this pull request Feb 24, 2016
[0.21] Commits to cherry-pick into 0.21-stable #6087
Closed
3 tasks

ghost closed this in ad8a335Mar 2, 2016

This pull request was closed.

Uh oh!

Conversation

skevy commented Jan 3, 2016

Uh oh!

facebook-github-bot commented Jan 3, 2016

Uh oh!

vjeux commented Jan 3, 2016

Uh oh!

vjeuxJan 3, 2016

Choose a reason for hiding this comment

Uh oh!

skevyJan 3, 2016

Choose a reason for hiding this comment

Uh oh!

martinbigioJan 3, 2016

Choose a reason for hiding this comment

Uh oh!

martinbigio commented Jan 3, 2016

Uh oh!

martinbigio commented Jan 3, 2016

Uh oh!

skevy commented Jan 3, 2016

Uh oh!

skevy commented Jan 3, 2016

Uh oh!

martinbigio commented Jan 4, 2016

Uh oh!

skevy commented Jan 4, 2016

Uh oh!

skevy commented Jan 5, 2016

Uh oh!

martinbigio commented Jan 5, 2016

Uh oh!

facebook-github-bot commented Jan 5, 2016

Uh oh!

davidaurelio commented Jan 5, 2016

Uh oh!

skevy commented Jan 5, 2016

Uh oh!

davidaurelio commented Jan 6, 2016

Uh oh!

skevy commented Jan 7, 2016

Uh oh!

skevy commented Jan 7, 2016

Uh oh!

facebook-github-bot commented Jan 7, 2016

Uh oh!

davidaurelio commented Jan 8, 2016

Uh oh!

skevy commented Jan 8, 2016

Uh oh!

davidaurelio commented Feb 23, 2016

Uh oh!

marcshilling commented Mar 1, 2016

Uh oh!

bestander commented Mar 1, 2016

Uh oh!

chirag04 commented Mar 1, 2016

Uh oh!

davidaurelio commented Mar 2, 2016

Uh oh!

ide commented Mar 3, 2016

Uh oh!

Kureev commented Mar 3, 2016

Uh oh!

chirag04 commented Mar 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants