[RFC] RN 0.18 Compatibility changes #95

skevy · 2016-01-03T01:38:26Z

This PR performs the necessary changes to fbjs to allow for better compatibility with React Native, post the facebook/react-native@6a838a4 commit.

TLDR; this PR does the following:

Strips the @providesModule directives from the built, npm version of fbjs (courtesy of @zpao). This is no longer necessary after [RFC] Remove knowledge of fbjs from the packager react-native#5084 is merged.
Switch to isomorphic-fetch, in order to avoid causing weird require issues that happen when the whatwg-fetch module is required on React Native (courtesy of @zpao). Also fixes the issues discussed in Switch to isomorphic-fetch #61, which is where this commit is from.
Adds the React Native promise implementation as Promise.native.js. The .native suffix was added in facebook/react-native@6a838a4, and allows the RN packager to require a different file for a given module.
Checks for existence of ErrorUtils before setting the fbjs version on global. This is needed as ErrorUtils on React Native is polyfilled with specific behavior.

Note: # 3 is most likely a temporary fix. After discussing with @spicyj, the desired result would be to eventually remove the packager complexity introduced in facebook/react-native@6a838a4. The reason I'm going for this approach at the moment is that it's only one module that needs this shimming in fbjs, and I think the value it brings, given the fact that this PR, along with the corresponding PR in react-native, allows for Relay to work OOTB, outweighs its small of amount of technical debt. It's very possible you all may disagree, and I'm happy to change the approach if we come up with a better solution. :)

yungsters · 2016-01-03T18:27:49Z

scripts/gulp/strip-provides-module.js

+      return;
+    }
+
+    // Get the @providesModule piece of out the file and save that.


Typo: out of

…eact Native compatibility.

yungsters · 2016-01-03T19:25:49Z

About to merge this, but wanted to double check. Have you tested this with the Relay examples from master to verify there aren't any regressions?

skevy · 2016-01-03T19:27:41Z

@yungsters I tested with my test Relay application...but let me test with the examples. I don't presume there will be any regressions, but I'll report back here in a bit.

skevy · 2016-01-03T20:34:03Z

Good news! It works! But, it brought to light test failures in Relay...fixed those in skevy/relay@b8fa29a

zpao · 2016-01-03T21:38:13Z

src/__forks__/fetch.js

+  module.exports = global.fetch.bind(global);
+} else {
+  module.exports = require('isomorphic-fetch');
+}


I never got around to revisiting the feedback about this in #61. If it works then great but I just wanted to make sure it was all good. Sounds like combined with the providesModule bits it might have fixed the issue.

It works fine, both on web and native.

The RN packager actually still pulls in isomorphic fetch, unfortunately, because it doesn't know about conditional requires. But, there's no error like there was with whatwg-fetch.

sophiebits · 2016-01-04T18:38:19Z

Note: Before merging this, we need a way to shim the 'warning' module, which we do internally at Facebook so that we can log the frequency of warnings in our dev environments. (YellowBox should probably go through the same codepath.)

skevy · 2016-01-04T19:08:11Z

@spicyj can you share how it's being shimmed now?

sophiebits · 2016-01-04T19:12:34Z

The module is blacklisted and we have another file with @providesModule warning that takes its place. I also have a @providesModule fbjs/lib/warning which just exports require('warning') which is necessary because that's how React requires it. It's hacky now.

skevy · 2016-01-04T19:16:53Z

@spicyj So, in RN then (post merging facebook/react-native#5084), in the FB internal blacklist, couldn't you still do the same thing? Blacklist fbjs/lib/warning and then do a module with @providesModule fbjs/lib/warning? Or, blacklist Libraries/vendor/fbjs/warning and do a module with @providesModule warning

sophiebits · 2016-01-04T19:20:56Z

Hm, maybe. It doesn't work if anyone in fbjs does a relative require of ./warning, but maybe that already doesn't work…

skevy · 2016-01-04T19:21:59Z

Yah, that's what I was thinking. I'm not sure if this PR or facebook/react-native#5084 actually change how you'd shim it -- besides making sure you guys are maintaining your own blacklist, which you already do anyway.

RN 0.18 Compatibility changes

skevy · 2016-01-05T01:45:44Z

Woo! Thanks @yungsters! You guys can close #61 and #84 now, as those were included here.

boourns · 2016-01-05T02:34:06Z

🎉

Summary: **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: 1. Remove the reliance on ReactDOM. The only use of ReactDOM is `unstable_batchedupdates`. So to fix, I abstracted the reference to `unstable_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. 2. Removed `react-dom` from peerDependencies (but kept it in devDependencies, for use in tests), and also upgrade the `fbjs` dependency to a (yet unreleased) version that provides better compatibility with React Native. Closes #713 Reviewed By: yungsters Differential Revision: D2872129 fb-gh-sync-id: f6ba6d06cfdde8ad8cbb0c7cd9d645f44f65e437

Summary: As spicyj mentioned in commit 6a838a4, the ideal state of affairs when it comes to consuming `react` and `fbjs` from NPM is for the packager not to have knowledge of either package. This PR addresses the `fbjs` part of that, and relies on facebook/fbjs#95. **DO NOT MERGE** until #95 (or a variation) is in `fbjs` and is released to npm. This PR does several things: 1. Adds stub modules within RN that expose `fbjs` modules to be required using Haste. After discussing a few ideas with spicyj, this seemed like a good option to keep internal FB devs happy (and not make them change the way they write JS), but allow for removing packager complexity and fit in better with the NPM ecosystem. Note -- it skips stubbing `fetch`, `ExecutionEnvironment`, and `ErrorUtils`, due to the fact that these need to have Native specific implementations, and there's no reason for those implementations to exist in `fbjs`. 2. Removes the modules that were previously being used in lieu of their `fbjs` eq Closes #5084 Reviewed By: bestander Differential Revision: D2803288 Pulled By: davidaurelio fb-gh-sync-id: fd257958ee2f8696eebe9048c1e7628c168bf4a2 shipit-source-id: fd257958ee2f8696eebe9048c1e7628c168bf4a2

Summary: As spicyj mentioned in commit 6a838a4, the ideal state of affairs when it comes to consuming `react` and `fbjs` from NPM is for the packager not to have knowledge of either package. This PR addresses the `fbjs` part of that, and relies on facebook/fbjs#95. **DO NOT MERGE** until #95 (or a variation) is in `fbjs` and is released to npm. This PR does several things: 1. Adds stub modules within RN that expose `fbjs` modules to be required using Haste. After discussing a few ideas with spicyj, this seemed like a good option to keep internal FB devs happy (and not make them change the way they write JS), but allow for removing packager complexity and fit in better with the NPM ecosystem. Note -- it skips stubbing `fetch`, `ExecutionEnvironment`, and `ErrorUtils`, due to the fact that these need to have Native specific implementations, and there's no reason for those implementations to exist in `fbjs`. 2. Removes the modules that were previously being used in lieu of their `fbjs` eq Closes #5084 Reviewed By: bestander Differential Revision: D2803288 Pulled By: javache fb-gh-sync-id: 121ae811ce4cc30e6ea79246f85a1e4f65648ce1 shipit-source-id: 121ae811ce4cc30e6ea79246f85a1e4f65648ce1

Summary: As spicyj mentioned in commit 6a838a4, the ideal state of affairs when it comes to consuming `react` and `fbjs` from NPM is for the packager not to have knowledge of either package. This PR addresses the `fbjs` part of that, and relies on facebook/fbjs#95. **DO NOT MERGE** until facebook#95 (or a variation) is in `fbjs` and is released to npm. This PR does several things: 1. Adds stub modules within RN that expose `fbjs` modules to be required using Haste. After discussing a few ideas with spicyj, this seemed like a good option to keep internal FB devs happy (and not make them change the way they write JS), but allow for removing packager complexity and fit in better with the NPM ecosystem. Note -- it skips stubbing `fetch`, `ExecutionEnvironment`, and `ErrorUtils`, due to the fact that these need to have Native specific implementations, and there's no reason for those implementations to exist in `fbjs`. 2. Removes the modules that were previously being used in lieu of their `fbjs` eq Closes facebook#5084 Reviewed By: bestander Differential Revision: D2803288 Pulled By: davidaurelio fb-gh-sync-id: fd257958ee2f8696eebe9048c1e7628c168bf4a2 shipit-source-id: fd257958ee2f8696eebe9048c1e7628c168bf4a2

Summary: As spicyj mentioned in commit 6a838a4, the ideal state of affairs when it comes to consuming `react` and `fbjs` from NPM is for the packager not to have knowledge of either package. This PR addresses the `fbjs` part of that, and relies on facebook/fbjs#95. **DO NOT MERGE** until facebook#95 (or a variation) is in `fbjs` and is released to npm. This PR does several things: 1. Adds stub modules within RN that expose `fbjs` modules to be required using Haste. After discussing a few ideas with spicyj, this seemed like a good option to keep internal FB devs happy (and not make them change the way they write JS), but allow for removing packager complexity and fit in better with the NPM ecosystem. Note -- it skips stubbing `fetch`, `ExecutionEnvironment`, and `ErrorUtils`, due to the fact that these need to have Native specific implementations, and there's no reason for those implementations to exist in `fbjs`. 2. Removes the modules that were previously being used in lieu of their `fbjs` eq Closes facebook#5084 Reviewed By: bestander Differential Revision: D2803288 Pulled By: javache fb-gh-sync-id: 121ae811ce4cc30e6ea79246f85a1e4f65648ce1 shipit-source-id: 121ae811ce4cc30e6ea79246f85a1e4f65648ce1

Summary: As spicyj mentioned in commit 6a838a4, the ideal state of affairs when it comes to consuming `react` and `fbjs` from NPM is for the packager not to have knowledge of either package. This PR addresses the `fbjs` part of that, and relies on facebook/fbjs#95. **DO NOT MERGE** until #95 (or a variation) is in `fbjs` and is released to npm. This PR does several things: 1. Adds stub modules within RN that expose `fbjs` modules to be required using Haste. After discussing a few ideas with spicyj, this seemed like a good option to keep internal FB devs happy (and not make them change the way they write JS), but allow for removing packager complexity and fit in better with the NPM ecosystem. Note -- it skips stubbing `fetch`, `ExecutionEnvironment`, and `ErrorUtils`, due to the fact that these need to have Native specific implementations, and there's no reason for those implementations to exist in `fbjs`. 2. Removes the modules that were previously being used in lieu of their `fbjs` eq Closes facebook/react-native#5084 Reviewed By: bestander Differential Revision: D2803288 Pulled By: davidaurelio fb-gh-sync-id: fd257958ee2f8696eebe9048c1e7628c168bf4a2 shipit-source-id: fd257958ee2f8696eebe9048c1e7628c168bf4a2

Summary: As spicyj mentioned in commit 6a838a4, the ideal state of affairs when it comes to consuming `react` and `fbjs` from NPM is for the packager not to have knowledge of either package. This PR addresses the `fbjs` part of that, and relies on facebook/fbjs#95. **DO NOT MERGE** until #95 (or a variation) is in `fbjs` and is released to npm. This PR does several things: 1. Adds stub modules within RN that expose `fbjs` modules to be required using Haste. After discussing a few ideas with spicyj, this seemed like a good option to keep internal FB devs happy (and not make them change the way they write JS), but allow for removing packager complexity and fit in better with the NPM ecosystem. Note -- it skips stubbing `fetch`, `ExecutionEnvironment`, and `ErrorUtils`, due to the fact that these need to have Native specific implementations, and there's no reason for those implementations to exist in `fbjs`. 2. Removes the modules that were previously being used in lieu of their `fbjs` eq Closes facebook/react-native#5084 Reviewed By: bestander Differential Revision: D2803288 Pulled By: javache fb-gh-sync-id: 121ae811ce4cc30e6ea79246f85a1e4f65648ce1 shipit-source-id: 121ae811ce4cc30e6ea79246f85a1e4f65648ce1

facebook-github-bot added the CLA Signed label Jan 3, 2016

This was referenced Jan 3, 2016

[RFC] Remove knowledge of fbjs from the packager facebook/react-native#5084

Closed

React Native Compatibility facebook/relay#713

Closed

Make Relay work with React Native out of the box facebook/relay#26

Closed

yungsters reviewed Jan 3, 2016
View reviewed changes

scripts/gulp/strip-provides-module.js

return;

}

// Get the @providesModule piece of out the file and save that.

Copy link

Contributor

yungsters Jan 3, 2016

Choose a reason for hiding this comment

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

Typo: out of

zpao and others added 4 commits January 3, 2016 14:04

Strip @providesModule directives

adc4328

Switch to isomorphic-fetch

488e033

Adding native Promise implementation (based on promise/setimmediate)

f4c92d5

Checking for ErrorUtils existence before setting. Allows for better R…

b8593c7

…eact Native compatibility.

skevy force-pushed the rn-0.18-compat branch from 3af2eb3 to b8593c7 Compare January 3, 2016 19:05

zpao reviewed Jan 3, 2016
View reviewed changes

Updating Promise.native.js license to BSD

7b58cbe

yungsters added a commit that referenced this pull request Jan 5, 2016

Merge pull request #95 from skevy/rn-0.18-compat

cb6c4cc

RN 0.18 Compatibility changes

yungsters merged commit cb6c4cc into facebook:master Jan 5, 2016

This was referenced Jan 5, 2016

Switch to isomorphic-fetch #61

Closed

Strip @providesModule directives #84

Closed

joewood mentioned this pull request Jan 29, 2016

Duplicate fbjs dependencies leading to "Error: Naming collision detected" facebook/react-native#5467

Closed

zpao mentioned this pull request Sep 28, 2018

Fix Promise.finally Non-standard behavior #293

Open

hramos mentioned this pull request Sep 28, 2018

Promise with "chained" finally gobbles return value facebook/react-native#17972

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] RN 0.18 Compatibility changes #95

[RFC] RN 0.18 Compatibility changes #95

skevy commented Jan 3, 2016

yungsters Jan 3, 2016

yungsters commented Jan 3, 2016

skevy commented Jan 3, 2016

skevy commented Jan 3, 2016

zpao Jan 3, 2016

skevy Jan 3, 2016

sophiebits commented Jan 4, 2016

skevy commented Jan 4, 2016

sophiebits commented Jan 4, 2016

skevy commented Jan 4, 2016

sophiebits commented Jan 4, 2016

skevy commented Jan 4, 2016

skevy commented Jan 5, 2016

boourns commented Jan 5, 2016

[RFC] RN 0.18 Compatibility changes #95

[RFC] RN 0.18 Compatibility changes #95

Conversation

skevy commented Jan 3, 2016

yungsters Jan 3, 2016

Choose a reason for hiding this comment

yungsters commented Jan 3, 2016

skevy commented Jan 3, 2016

skevy commented Jan 3, 2016

zpao Jan 3, 2016

Choose a reason for hiding this comment

skevy Jan 3, 2016

Choose a reason for hiding this comment

sophiebits commented Jan 4, 2016

skevy commented Jan 4, 2016

sophiebits commented Jan 4, 2016

skevy commented Jan 4, 2016

sophiebits commented Jan 4, 2016

skevy commented Jan 4, 2016

skevy commented Jan 5, 2016

boourns commented Jan 5, 2016