Move React Core Integration and Injection to the Core Repo #6338

sebmarkbage · 2016-03-25T02:11:58Z

This creates a new react-native-renderer package which contains the React integration for React Native. (Although in reality the files live in the react package just like for react-dom because builds.)

I moved the most core files over but I still want to see how we can further minimize the contract. The individual commits show the process.

Currently everything in src/renderers/native/ReactNative/__mocks__/ have to required from the platform.

This also contains some patches to make this compatible with 15.0. As well as enabling spread properties.

I still need to run further test in the React Native repo to ensure everything works as expected.

We might want to consider this as part of 15.0 to avoid publishing a 15.1 immediately after since we'd have to do a minor bump for this stuff.

iamdustan · 2016-03-25T02:15:58Z

I’m curious if you’ve considered doing the inverse of this and moving the DOM renderer out to it’s own repo rather than consolidating the FB renderers in here?

sebmarkbage · 2016-03-25T02:20:06Z

Once we have a more stable contract for renderers we might do that but currently it is unstable. It is too hard to iterate on the internals and keep them in sync when they're in separate repos. It is also too easy to create massive cross dependencies.

The idea isn't that these files will remain RN specific but to make more of this stuff shared and narrow the external contract.

iamdustan · 2016-03-25T02:36:51Z

Copy that. The React external contract is already so solid. It will be an amazing feat of engineering and API design whenever React gets to the point where the custom renderer contract is as stable and small as the public API.

iamdustan · 2016-03-25T02:38:13Z

packages/react-native-renderer/package.json

+  },
+  "homepage": "https://facebook.github.io/react-native/",
+  "dependencies": {
+    "fbjs": "0.1.0-alpha.4",


I’m assuming this is copied out of RN? The current release listed on https://www.npmjs.com/package/fbjs is v0.8.0-alpha.3

good call. This was actually long before fbjs was in RN. I should update this. @zpao Your warning didn't catch this. :)

facebook-github-bot · 2016-03-25T02:51:27Z

@sebmarkbage updated the pull request.

zpao · 2016-03-25T17:32:21Z

src/renderers/native/ReactNative/ReactNativeBaseComponent.js

+var UIManager = require('UIManager');
+
+var deepFreezeAndThrowOnMutationInDev = require('deepFreezeAndThrowOnMutationInDev');
+var invariant = require('fbjs/lib/invariant');


Pretty sure these are going to get rewritten to './fbjs/lib/invariant', might just want to make them 'invariant' for now.

hehe. I should've pushed last night. Fixed this locally.

zpao · 2016-03-25T17:43:42Z

We might want to consider this as part of 15.0

2 RCs deep is not really the time to drop this…

Is there a less aggressive thing we can do to make sure RN works with 15 and do something like this for 15.1? Also, looks like this is purely new stuff and doesn't touch any of our existing code that would be in the React package. Could we perhaps just ship React 15 without needing this and then clean this up and ship the new package, like next week.

Also also, how is this going to affect our internal syncing?

sebmarkbage · 2016-03-25T18:45:17Z

I'm still working on compatibility with RN. There are a lot of things broken with 15 in RN. That's exactly why we need to fix this since we've gone way too long without making it RN compatible.

Not sure yet if that would need a new release of react to fix it regardless.

Ideally we should only have to release a new version of react-native-renderer to do these fixes, but we can't because everything still actually lives in react and we don't have flat bundles that copies the relevant shared code to each renderer package. They still share the same modules.

zpao · 2016-03-25T18:48:46Z

we can't because everything still actually lives in react

Oh right… that.

zpao · 2016-03-25T19:01:45Z

I'm still working on compatibility with RN. There are a lot of things broken with 15 in RN

Honestly, that makes me want to push to 15.1 even more. Do you expect any core API changes to accommodate RN? As long as our core API doesn't change and we wouldn't need to ship a v16, I'm leaning strongly towards just saying we get 15.0 out and do what ever else we need in 15.1, even if that happens relatively soon after. We've been sitting on this release for too long already and I don't really want to push a couple more weeks & do more RCs. That's the nice thing about having these major version numbers :)

sebmarkbage · 2016-03-25T19:06:24Z

Also also, how is this going to affect our internal syncing?

We might need to stub out the missing files to satisfy the module systems but other than that it shouldn't affect us. We can just pull in the files that won't be required.

As long as our core API doesn't change and we wouldn't need to ship a v16

I don't think we'll need to make it 16. We can't make it a patch though.

The minor could potentially break other renderers like React ART since, again, they still share the same internal modules. If that's the case it is arguable that it would need to be a v16 release.

facebook-github-bot · 2016-03-27T20:38:41Z

@sebmarkbage updated the pull request.

facebook-github-bot · 2016-03-30T21:01:08Z

@sebmarkbage updated the pull request.

facebook-github-bot · 2016-04-05T08:12:04Z

@sebmarkbage updated the pull request.

facebook-github-bot · 2016-04-05T08:19:33Z

@sebmarkbage updated the pull request.

This has a different file name from its providesModule. Screws up our build scripts.

Changes to event overloading structure

...even though they technically still are attached by.

This is the new protocol.

This can happen in edge cases where he listeners are already unmounted or not mounted yet or something.

This isn't actually used right now so I can't test it. Because the Chrome devtools are broken for React Native. The Nuclide integration is in the react-native repo.

This is causing build errors. This should be in the downstream repo if anything. Relay has its own shim that should be preferred.

facebook-github-bot · 2016-04-20T02:52:14Z

@sebmarkbage updated the pull request.

Clean up package.json after #6338

Move React Core Integration and Injection to the Core Repo (cherry picked from commit c84ad52)

Clean up package.json after #6338 (cherry picked from commit bddecc9)

Summary:Adding the react native renderer dependency and various fixes to support React 15. Don't use dispatchID for touchableHandleResponderGrant This callback argument was removed because "IDs" no longer exist. Instead, we'll use the tag from the event target. The corresponding PR on React Core is: facebook/react#6338 Reviewed By: spicyj Differential Revision: D3159788 fb-gh-sync-id: 60e5cd2aa0af69d83fcdac3dfde0a85a748cb7b9 fbshipit-source-id: 60e5cd2aa0af69d83fcdac3dfde0a85a748cb7b9

sebmarkbage added this to the 15.0 milestone Mar 25, 2016

facebook-github-bot added GH Review: review-needed CLA Signed labels Mar 25, 2016

iamdustan reviewed Mar 25, 2016
View reviewed changes

sebmarkbage force-pushed the reactnative2 branch from 08d9350 to 2b7128b Compare March 25, 2016 02:50

zpao reviewed Mar 25, 2016
View reviewed changes

This was referenced Mar 26, 2016

Make ResponderEventPlugin cross-platform #6352

Closed

Make ResponderEventPlugin work with DOM renderer (not in working state) #4303

Closed

sebmarkbage force-pushed the reactnative2 branch from 2b7128b to ad03e18 Compare March 27, 2016 20:37

zpao modified the milestones: 15.x, 15.0 Mar 30, 2016

sebmarkbage force-pushed the reactnative2 branch from ad03e18 to 5d7771a Compare March 30, 2016 21:36

sebmarkbage force-pushed the reactnative2 branch from 61bbb0f to ddd395d Compare April 5, 2016 08:12

sebmarkbage added 12 commits April 20, 2016 03:35

Fix 0.15 compatibility

f833516

Fix ReactNativeComponentEnvironment providesModule

97b079b

This has a different file name from its providesModule. Screws up our build scripts.

Get rid of rootNodeIDs they're just tags now

75cec60

Build up a native component cache for event dispatching

3287d93

Changes to event overloading structure

Listeners are not attached by ID in the API

4fcdf02

...even though they technically still are attached by.

Update ReactNativeDOMIDOperations to deal with a single parent node

dc188dd

This is the new protocol.

Bump package versions

0640921

Don't try to get the target node for responder if there is no listener

22a8f99

This can happen in edge cases where he listeners are already unmounted or not mounted yet or something.

Ensure react-native-renderer package gets copied

c7d90e2

Update devtools injection

5470972

This isn't actually used right now so I can't test it. Because the Chrome devtools are broken for React Native. The Nuclide integration is in the react-native repo.

Fix lint for moved ReactNative

744548a

Remove ReactDOM.native shim

ada60c4

This is causing build errors. This should be in the downstream repo if anything. Relay has its own shim that should be preferred.

sebmarkbage force-pushed the reactnative2 branch from 35239c4 to ada60c4 Compare April 20, 2016 02:35

sebmarkbage merged commit c84ad52 into facebook:master Apr 20, 2016

zpao added a commit to zpao/react that referenced this pull request Apr 20, 2016

Clean up package.json after facebook#6338

9354751

zpao modified the milestones: 15.0.x, 15.x Apr 20, 2016

zpao added a commit that referenced this pull request Apr 20, 2016

Merge pull request #6553 from zpao/cleanuppkg

bddecc9

Clean up package.json after #6338

zpao pushed a commit that referenced this pull request Apr 20, 2016

Merge pull request #6338 from sebmarkbage/reactnative2

5b4b6e7

Move React Core Integration and Injection to the Core Repo (cherry picked from commit c84ad52)

zpao added a commit that referenced this pull request Apr 20, 2016

Merge pull request #6553 from zpao/cleanuppkg

c81c29a

Clean up package.json after #6338 (cherry picked from commit bddecc9)

zpao added the semver-patch label Apr 22, 2016

zpao modified the milestones: 15.0.2, 15.0.x Apr 28, 2016

MikeOrtiz mentioned this pull request May 2, 2016

Fix ReactNativeART arc drawing on Android facebook/react-native#7049

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move React Core Integration and Injection to the Core Repo #6338

Move React Core Integration and Injection to the Core Repo #6338

sebmarkbage commented Mar 25, 2016

iamdustan commented Mar 25, 2016

sebmarkbage commented Mar 25, 2016

iamdustan commented Mar 25, 2016

iamdustan Mar 25, 2016

sebmarkbage Mar 25, 2016

facebook-github-bot commented Mar 25, 2016

zpao Mar 25, 2016

sebmarkbage Mar 25, 2016

zpao commented Mar 25, 2016

sebmarkbage commented Mar 25, 2016

zpao commented Mar 25, 2016

zpao commented Mar 25, 2016

sebmarkbage commented Mar 25, 2016

facebook-github-bot commented Mar 27, 2016

facebook-github-bot commented Mar 30, 2016

facebook-github-bot commented Apr 5, 2016

facebook-github-bot commented Apr 5, 2016

facebook-github-bot commented Apr 20, 2016

Move React Core Integration and Injection to the Core Repo #6338

Move React Core Integration and Injection to the Core Repo #6338

Conversation

sebmarkbage commented Mar 25, 2016

iamdustan commented Mar 25, 2016

sebmarkbage commented Mar 25, 2016

iamdustan commented Mar 25, 2016

iamdustan Mar 25, 2016

Choose a reason for hiding this comment

sebmarkbage Mar 25, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Mar 25, 2016

zpao Mar 25, 2016

Choose a reason for hiding this comment

sebmarkbage Mar 25, 2016

Choose a reason for hiding this comment

zpao commented Mar 25, 2016

sebmarkbage commented Mar 25, 2016

zpao commented Mar 25, 2016

zpao commented Mar 25, 2016

sebmarkbage commented Mar 25, 2016

facebook-github-bot commented Mar 27, 2016

facebook-github-bot commented Mar 30, 2016

facebook-github-bot commented Apr 5, 2016

facebook-github-bot commented Apr 5, 2016

facebook-github-bot commented Apr 20, 2016