-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
React Native Compatibility #713
Conversation
Awesome, thanks for working on this! |
💯 |
👍 👏 |
@@ -0,0 +1,19 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this file be named RelayUnstableBatchedUpdates.native.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it can, and we can get rid of what's in package.json...because the RN packager looks for the native extension. Though @spicyj has mentioned that he'd like to remove that eventually. I suppose if he does, we can just put back the package.json directive.
This is great — thanks for tackling it. And great write ups, too. |
Use the proper unstable_batchedupdates for each platform
Do you suppose there's any chance to pull this |
@skevy this is now blocked only on facebook/react-native#5084, right? |
@josephsavona yah. Hopefully being merged soon! :) |
Fyi @josephsavona, fbjs 0.7.0 is now released, so presuming you don't have any changes on this PR, it can be merged :) |
Would also be cool if you could release relay@0.6.2 -- nothing in here is breaking, so it should keep in line with semver. |
@facebook-github-bot import |
@@ -85,6 +85,7 @@ | |||
"<rootDir>/src/" | |||
], | |||
"unmockedModulePathPatterns": [ | |||
"<rootDir>/src/tools/RelayUnstableBatchedUpdates.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm assuming a manual mock with module.exports = require.requireActual('...')
won't work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'm not sure how to do that :( Sorry...not a Jest expert. If you point me in the right direction, happy to make the change...otherwise I defer to your decision on that.
Based on my limited knowledge of requireActual...it seems like it would work fine. But I'm not 100% certain.
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1014651835240270/int_phab to review. |
60c8a76
This PR depends on an unreleased version of
fbjs
, so DO NOT MERGE.When merged along with facebook/react-native#5084, facebook/fbjs#95, and whatever PR fixes facebook/react-native#4062 (which I will update this issue with when I push it), this fixes #26.
The changes to Relay itself are super minor here:
unstable_batchedupdates
. So to fix, I abstracted the reference tounstable_batchedupdates
to it's own module, and then took advantage of the "react-native"package.json
option, supported by the React Native packager, to load the correct version of this function given the execution context.react-dom
from peerDependencies (but kept it in devDependencies, for use in tests), and also upgrade thefbjs
dependency to a (yet unreleased) version that provides better compatibility with React Native.