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

Ensure Glamorous forward props to RCTView rootEl #85

Merged
merged 1 commit into from Dec 19, 2017

Conversation

Projects
None yet
3 participants
@slorber
Contributor

slorber commented Dec 12, 2017

For undocumented reasons, importing View in recent versions of RN (RN50/Expo SDK23) does return string "RCTView" in prod, while in dev it returns a function.

It is the only component in RN codebase that behaves this way in prod build.

See for example ReactNativeElementMap log output:

17:17:30: ReactNativeElementMap Object {
17:17:30:   "FlatList": [Function FlatList],
17:17:30:   "Image": [Function anonymous],
17:17:30:   "ListView": [Function anonymous],
17:17:30:   "ScrollView": [Function anonymous],
17:17:30:   "SectionList": [Function SectionList],
17:17:30:   "Text": [Function anonymous],
17:17:30:   "TextInput": [Function anonymous],
17:17:30:   "TouchableHighlight": [Function anonymous],
17:17:30:   "TouchableNativeFeedback": [Function anonymous],
17:17:30:   "TouchableOpacity": [Function anonymous],
17:17:30:   "TouchableWithoutFeedback": [Function anonymous],
17:17:30:   "View": "RCTView",
17:17:30: }

Some code references in RN codebase that looks related:

https://github.com/facebook/react-native/blob/64d80b13db3dd28bb5d93cbedf511ae8f91e2127/Libraries/Components/View/View.js#L95
facebook/react-native@9344f3a#diff-a562e4a7ac9e900c6d2bc644453e9152

should fix:

#84
#83
#81

Ensure Glamorous forward props to RCTView rootEl
For undocumented reasons, importing View in recent versions of RN (RN50/Expo SDK23) does return string "RCTView" in prod, while in dev it returns a function.

It is the only component in RN codebase that behaves this way in prod build.

See for example ReactNativeElementMap log output:

```
17:17:30: ReactNativeElementMap Object {
17:17:30:   "FlatList": [Function FlatList],
17:17:30:   "Image": [Function anonymous],
17:17:30:   "ListView": [Function anonymous],
17:17:30:   "ScrollView": [Function anonymous],
17:17:30:   "SectionList": [Function SectionList],
17:17:30:   "Text": [Function anonymous],
17:17:30:   "TextInput": [Function anonymous],
17:17:30:   "TouchableHighlight": [Function anonymous],
17:17:30:   "TouchableNativeFeedback": [Function anonymous],
17:17:30:   "TouchableOpacity": [Function anonymous],
17:17:30:   "TouchableWithoutFeedback": [Function anonymous],
17:17:30:   "View": "RCTView",
17:17:30: }
```


Some code references in RN codebase that looks related:

https://github.com/facebook/react-native/blob/64d80b13db3dd28bb5d93cbedf511ae8f91e2127/Libraries/Components/View/View.js#L95
facebook/react-native@9344f3a#diff-a562e4a7ac9e900c6d2bc644453e9152
@codecov-io

This comment has been minimized.

codecov-io commented Dec 12, 2017

Codecov Report

Merging #85 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
+ Coverage    92.3%   92.36%   +0.05%     
==========================================
  Files          11       11              
  Lines         143      144       +1     
  Branches       40       41       +1     
==========================================
+ Hits          132      133       +1     
  Misses          7        7              
  Partials        4        4
Impacted Files Coverage Δ
src/split-props.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68a6250...eeca934. Read the comment docs.

@ajwhite

This comment has been minimized.

Member

ajwhite commented Dec 12, 2017

Wow! Great discovery @slorber!

This makes a lot more sense why RN no longer lets you use View.propTypes and have to import ViewPropTypes instead.. 💡

I think we may need to make a patch here in isStyleProp. Normally we get a reference to the View component, and we can see if it exists in the collection of components that inherit ViewStylePropTypes.

I think if we add 'RCTView' to viewStyleComponents, it'll properly pick up what style rules are supported by views in production. Otherwise I think isStyleProp would return false in production builds

@ajwhite ajwhite self-requested a review Dec 12, 2017

@ajwhite ajwhite added the bug label Dec 12, 2017

@ajwhite

This comment has been minimized.

Member

ajwhite commented Dec 12, 2017

Oh man, what a rabbit hole this was.

So I was very confused how 'RCTView' was a string, when it appears to be some instance of a React component.

As it turns out, requireNativeComponent leads returning createReactNativeComponentClass. Hrmm... still seems like it should not be a string, but instead should be some class definition...

Okay, so let's look at the definition of `createReactNativeComponentClass.. Not helpful, (subtle "WTF" moment as I opened that file).

Onto finding __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED, which appears to be defined in a -prod flagged file ReactNativeRenderer-prod.js. A little further down, sure enough createReactNativeComponentClass is found, and it returns the string name that was provided to it. It has a side effect of registering that name with some callbacks that seem to link the name to, perhaps, a class definition.

In the end, we sure enough get a string, and it now makes sense why that string only appears in production.

oof.

@slorber

This comment has been minimized.

Contributor

slorber commented Dec 12, 2017

:) nooo i don't want to be fired

'RCTView' is already in the array as it's the View element itself ;)

@ajwhite

This comment has been minimized.

Member

ajwhite commented Dec 12, 2017

That.. would make sense 😛

@ajwhite

This comment has been minimized.

Member

ajwhite commented Dec 12, 2017

Text is mentioned here too, https://twitter.com/brian_d_vaughn/status/940645433260384256, but looks like we are safe.

Nothing else I can think of that we need to do here, can you? I think we're good to go

@slorber

This comment has been minimized.

Contributor

slorber commented Dec 12, 2017

I don't know, but this PR fix is already shipped to my production app. I think it's the only thing required for now but it may not be the most future proof solution.

I've had a difficult day today, first publication of our Expo app after detaching. 3 things breaking only after production deployment :)

@slorber

This comment has been minimized.

Contributor

slorber commented Dec 13, 2017

@ajwhite if you can merge and make a release asap that would help me :)

@ajwhite ajwhite merged commit d4b254d into robinpowered:master Dec 19, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ajwhite

This comment has been minimized.

Member

ajwhite commented Dec 19, 2017

Will publish tomorrow AM 👍

@slorber

This comment has been minimized.

Contributor

slorber commented Dec 19, 2017

great thanks ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment