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

[RN] Move view config registry to shims #12569

Merged
merged 2 commits into from
Apr 10, 2018

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Apr 7, 2018

Builds on top of #12556

This ensures that both Fabric and RN renderers share the same view config registry since it is stateful.

I had to duplicate in the mocks for testing.

@sophiebits
Copy link
Collaborator

I don't understand why it's duplicated or what difference being in the shims folder makes.

@sophiebits
Copy link
Collaborator

…and that's probably why you didn't tag me as reviewer. Oops.

@sebmarkbage
Copy link
Collaborator Author

Shims is the only thing we have set up that can be standalone files without being bundled. The alternative is building a new bundle that only has this file in it but I think a more likely set up is that we just move this file to RN and keep the mock so it works like all the other external deps.

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Apr 8, 2018

There's also other reasons to put it in shims. Because we have a bunch of lint rules and stuff set up that means you can't do things like use CommonJS in normal src files. It's quite difficult to not bundle something.

@sebmarkbage
Copy link
Collaborator Author

I also moved createReactNativeComponentClass since it's just an alias for the registry. It should really just move back to RN.

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

This change looks okay to me, but why can't we just remove these from the React repo entirely now? The shims are only useful if they're in place to forward to "SECRET INTERNALS" stuff, right?

}
invariant(viewConfig, 'View config not found for name %s', name);
return viewConfig;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

We should just move extractEvents out of ReactNativeBridgeEventPlugin and into this file too so we can get rid of ReactNativeBridgeEventPlugin. 😄

This ensures that both Fabric and RN renderers share the same view config
registry since it is stateful.

I had to duplicate in the mocks for testing.
Since createReactNativeComponentClass is just an alias for the register
there's no need to bundle it. This file should probably just move back
to RN too.
@sebmarkbage sebmarkbage merged commit b99d0b1 into facebook:master Apr 10, 2018
rhagigi pushed a commit to rhagigi/react that referenced this pull request Apr 19, 2018
* Move view config registry to shims

This ensures that both Fabric and RN renderers share the same view config
registry since it is stateful.

I had to duplicate in the mocks for testing.

* Move createReactNativeComponentClass to shims and delete internal usage

Since createReactNativeComponentClass is just an alias for the register
there's no need to bundle it. This file should probably just move back
to RN too.
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