[WIP] ReactNativeFiber mixins refactor #9013

bvaughn · 2017-02-15T21:38:51Z

Note This PR has changed significantly since it was initially created. (See commit history for more.)

Addressed a circular dependency between NativeMethodsMixin and ReactNativeFiber:

Created new ReactNativeFiberHostComponent with similar interface but without unnecessary call to findNodeHandle.
Created new Flow interface for mixins to ensure ReactNativeFiberHostComponent and NativeMethodsMixinUtils stay synced.
Moved helper methods to NativeMethodsMixinUtils to be shared between the 2 wrappers.

Fixed bug in createReactNativeComponentClass that allowed duplicate view names to be registered and added a unit test to guard against a regression.

sebmarkbage · 2017-02-15T21:49:37Z

We actually have a task to undo all the tests that reach into internals like this. :) It makes refactoring harder.

You can express this using public APIs. You can test the first two by calling createReactNativeComponentClass.

You can test the last one by rendering a view like <foo /> using the renderer.

bvaughn · 2017-02-15T21:54:09Z

Yeah, I considered testing with createReactNativeComponentClass but the behavior varies between fiber and stack. (Stack doesn't error.)

sebmarkbage · 2017-02-15T21:55:50Z

That's an argument for doing it so that we're aware of the differences. It might actually be a problem for us because people copy paste stuff into their own modules. Not guaranteed to have unique names. Would be better with an incremental counter.

sebmarkbage

We can add a test for the public API behind a feature flag like we've done in other places if needed.

I don't think this is the strategy we should go with at all though since it can break the ecosystem when people load different modules. Worse case you load modules dynamically. If you test each feature in isolation you get no error. Then the end user visits both features in production and you get an error.

sebmarkbage · 2017-02-15T23:25:14Z

I suppose it's unlikely that you have two different modules targeting the same native uiViewClassName but it's possible.

bvaughn · 2017-02-15T23:29:33Z

I don't think this is the strategy we should go with at all though since it can break the ecosystem when people load different modules.

Yes, I agree. We could switch to some other uid (like a counter) to make this particular issue more robust, but there's also the related issue of native mixins so maybe this is a better time to re-think the return value of ReactNativeViewConfigRegistry.register

Let's sync up tomorrow at the office and talk more.

gaearon · 2017-02-16T12:33:02Z

src/renderers/shared/fiber/ReactFiber.js

-    fiber.type = type;
+    // @TODO (bvaughn) This is temporary hack just to get things working.
+    if (typeof type.__reactInternalHostComponentFlag !== 'undefined') {
+      fiber = createFiber(HostComponent, key);


I hope this doesn't stay for a couple of years like similar hacks have stayed in Stack :-)

What is the plan for unhacking this?

This PR is just me poking things a bit and familiarizing myself with the problem. Sebastian and I are going to talk more about it when he gets in this morning. 😄

Not sure if that one gets merged but it fixes other issues so it's likely.

janicduplessis · 2017-02-19T09:33:36Z

There was a circular dependency that was causing a crash when running the Native Animated example in UIExplorer with Fiber, ReactNative -> NativeModulesMixin -> ReactNative. I was about to send a fix but I think this PR will also fix it.

…eturn wrapper object (like stack does) instead of string. This is a work-in-progress. Pushing now for sharing purposes.

…ativeFiber: • Created new ReactNativeFiberHostComponent with similar interface but without unnecessary call to findNodeHandle. • Created new Flow interface for mixins to ensure ReactNativeFiberHostComponent and NativeMethodsMixinUtils stay synced. • Moved helper methods to NativeMethodsMixinUtils to be shared between the 2 wrappers. Added createReactNativeComponentClass test to ensure that ReactNativeFiber doesn't permit conflicting native view configs to be registered.

This method depends on viewConfig, which used to be an instance property copied from the RN host component type. Now that this type is a string the viewConfig property is undefined for class components. The mixed-in setNativeProps method gets the view config from the nearest host component, which happens to also be the one the properties are set on.

bvaughn · 2017-03-08T18:37:20Z

I've moved this effort to an internal diff for now because it's easier to test its impact that way. I'm going to close this PR (since it's changed so much from the initial commit anyway) and I'll open a new one once the internal diff has been merged. 😄

gaearon · 2017-03-08T18:41:39Z

I'll open a new one once the internal diff has been merged

Wouldn't it just show up as a commit then? I don't think you need to do anything once internal diff lands.

gaearon · 2017-03-08T18:42:26Z

Oh, we're in React repo. Nevermind 😄

Not sure if that one gets merged but it fixes other issues so it's likely.

* Fix a crash due to facebook/react#9013 Not sure if that one gets merged but it fixes other issues so it's likely. * Fix editing host styles in RN Also switch it to use the same `updater` code path both in Stack and Fiber. This removes knowledge about internal data structure from the frontend. * Fully fix editing RN styles * We shouldn't rely on the assumption that we're operating on current Fiber * The idea to unify updaters with setInProps() was a bad one because setNativeProps() signature is too different * Flow fixes; I think the correct thing to do is to use Dom's newer code over Dan's older code for setupBackend.

bvaughn requested a review from sebmarkbage February 15, 2017 21:38

facebook-github-bot added GH Review: review-needed CLA Signed labels Feb 15, 2017

sebmarkbage requested changes Feb 15, 2017

View reviewed changes

bvaughn force-pushed the ReactNativeViewConfigRegistry-invariant-check-fix branch from 8a3f00a to e2a0152 Compare February 16, 2017 00:42

bvaughn changed the title ~~Fixed a bad invariant check in ReactNativeViewConfigRegistry~~ Refactor how viewConfigs are handled for ReactNativeFiber Feb 16, 2017

bvaughn changed the title ~~Refactor how viewConfigs are handled for ReactNativeFiber~~ [WIP] Refactor how viewConfigs are handled for ReactNativeFiber Feb 16, 2017

gaearon reviewed Feb 16, 2017

View reviewed changes

gaearon added a commit to gaearon/react-devtools that referenced this pull request Feb 16, 2017

Fix a crash due to facebook/react#9013

ce449d5

Not sure if that one gets merged but it fixes other issues so it's likely.

bvaughn force-pushed the ReactNativeViewConfigRegistry-invariant-check-fix branch 2 times, most recently from 38ff586 to f451cd2 Compare February 16, 2017 18:30

Brian Vaughn added 2 commits February 28, 2017 12:47

Refactor fiber implementation of createReactNativeComponentClass to r…

111c928

…eturn wrapper object (like stack does) instead of string. This is a work-in-progress. Pushing now for sharing purposes.

Bumped Flow from v0.37.0 to v0.38.0 because I want to use implements

63d031c

bvaughn force-pushed the ReactNativeViewConfigRegistry-invariant-check-fix branch from f451cd2 to b5e9614 Compare February 28, 2017 21:16

bvaughn force-pushed the ReactNativeViewConfigRegistry-invariant-check-fix branch from b5e9614 to ef36bf8 Compare February 28, 2017 21:23

bvaughn changed the title ~~[WIP] Refactor how viewConfigs are handled for ReactNativeFiber~~ [WIP] ReactNativeFiber mixins refactor Feb 28, 2017

sebmarkbage removed the GH Review: review-needed label Mar 1, 2017

bvaughn closed this Mar 8, 2017

bvaughn deleted the ReactNativeViewConfigRegistry-invariant-check-fix branch March 27, 2017 15:41

bvaughn pushed a commit to facebook/react-devtools that referenced this pull request May 30, 2017

Fix a crash due to facebook/react#9013

b15a34f

Not sure if that one gets merged but it fixes other issues so it's likely.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] ReactNativeFiber mixins refactor #9013

[WIP] ReactNativeFiber mixins refactor #9013

Uh oh!

bvaughn commented Feb 15, 2017 •

edited

Loading

Uh oh!

sebmarkbage commented Feb 15, 2017

Uh oh!

bvaughn commented Feb 15, 2017

Uh oh!

sebmarkbage commented Feb 15, 2017

Uh oh!

sebmarkbage left a comment

Uh oh!

sebmarkbage commented Feb 15, 2017

Uh oh!

bvaughn commented Feb 15, 2017

Uh oh!

gaearon Feb 16, 2017

Uh oh!

gaearon Feb 16, 2017

Uh oh!

bvaughn Feb 16, 2017

Uh oh!

janicduplessis commented Feb 19, 2017 •

edited

Loading

Uh oh!

bvaughn commented Mar 8, 2017

Uh oh!

gaearon commented Mar 8, 2017

Uh oh!

gaearon commented Mar 8, 2017

Uh oh!

Uh oh!

[WIP] ReactNativeFiber mixins refactor #9013

[WIP] ReactNativeFiber mixins refactor #9013

Uh oh!

Conversation

bvaughn commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebmarkbage commented Feb 15, 2017

Uh oh!

bvaughn commented Feb 15, 2017

Uh oh!

sebmarkbage commented Feb 15, 2017

Uh oh!

sebmarkbage left a comment

Choose a reason for hiding this comment

Uh oh!

sebmarkbage commented Feb 15, 2017

Uh oh!

bvaughn commented Feb 15, 2017

Uh oh!

gaearon Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

gaearon Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

bvaughn Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

janicduplessis commented Feb 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvaughn commented Mar 8, 2017

Uh oh!

gaearon commented Mar 8, 2017

Uh oh!

gaearon commented Mar 8, 2017

Uh oh!

Uh oh!

bvaughn commented Feb 15, 2017 •

edited

Loading

janicduplessis commented Feb 19, 2017 •

edited

Loading