Validate that JS and Native code versions match for RN releases #15518

janicduplessis · 2017-08-16T20:29:29Z

Basic implementation of the proposal in #15271

Note that this should not affect facebook internally since they are not using OSS releases.

Points to consider:

How strict should the version match be, right now I just match exact versions.
Wasn't able to use haste for ReactNativeVersion because I was getting duplicate module provider caused by the template file in scripts/versiontemplates. I tried adding the scripts folder to modulePathIgnorePatterns in package.json but that didn't help.
Redscreen vs. warning, I think warning is useless because if the app crashes you won't have time to see the warning.
Should the check and native modules be DEV only?

Test plan
Tested that it works when version match and that it redscreens when versions don't before getting other errors on Android and iOS.

janicduplessis · 2017-08-16T20:29:48Z

cc @ide @grabbou

pull-bot · 2017-08-16T20:37:04Z

@facebook-github-bot label Core Team

Attention: @shergin

Generated by 🚫 dangerJS

ide · 2017-08-17T00:34:42Z

Version match: I'd really like to make this check be configurable and have a default implementation that checks for an exact match but also let people override it and check only the major and minor versions if they wish. The difference between patch releases usually doesn't include breaking changes (just new native code or new JS that doesn't require the JS and native to match) and it would be nice to let people choose if they're OK with 0.46.1 JS running on 0.46.0 native.
Haste: I wonder if the issue is that modulePathIgnorePatterns applies only to Jest but not Metro. I think your workaround is simple and fine, otherwise we could make the templates not contain @providesModule and make the template generator add it in.
Throwing vs warning: let's try throwing for now so we fail fast.
About __DEV__: let's keep the check in production. It's cheap in several ways (bytes & runtime) and if there's a version mismatch in prod that results in errors like "X is not a callable module", that's way harder to diagnose than "You're using the wrong version of RN." If we make it a __DEV__-only check, I ask that it does console.error instead of throwing an error so we don't change code paths in production.

javache · 2017-08-25T11:08:12Z

I'm not sure I agree with this direction. It could be useful at development time, but we're actually trying to keep the JS compatible with older native code versions for as long as possible, and this would actually prevent that.

ide · 2017-08-25T18:04:50Z

@javache what is the mechanism you use to keep the JS compatible with the native code? Could we use that instead of semver? Also this code doesn't materially affect FB (or anyone else who runs from master since the compared version is always "master") in case that affects your thinking around this.

javache · 2017-08-29T09:57:06Z

True, since we don't use the release branch, this wouldn't necessarily affect us. I would still prefer not having an additional native module, so perhaps this can go in PlatformConstants ?

Internally, we're mainly looking at leveraging flow for typing native interfaces and end-to-end testing to ensure compatibility.

ide · 2017-08-29T20:48:14Z

@javache Sounds good, let's move this into PlatformConstants. Native -> Flow defs sounds useful, I think doing the version check will also catch a different class of issues caused by old packager processes, cached bundles, etc...

janicduplessis · 2017-09-01T23:03:21Z

@ide What kind of API were you thinking of to customize that check? It might be a bit tricky since I'm not sure it's even possible to run code before the InitializeCore module.

janicduplessis · 2017-09-01T23:18:37Z

@ide Just tested and InitializeCore does get executed before the app index.js, so I don't see a way to customize this behavior at runtime. I'd also like to keep the check in InitializeCore since otherwise other random errors could happen before the version check.

ide · 2017-09-02T06:06:03Z

OK, that makes sense. This looks good to me then.

@javache How does this look to you?

ide · 2017-09-09T17:47:13Z

scripts/bump-oss-version.js

  exit(1);
 }

+// Generate version files to detect mismatches between JS and native.
+let [, major, minor, patch, prerelease] = version.match(/^(\d+)\.(\d+)\.(\d+)(?:-rc\.(\d+))?$/);


Prerelease should be a string (not number) that includes -beta.1, etc... can you change the regex to this?

/^(\d+)\.(\d+)\.(\d+)(?:-(.+))?$/

Also for clearer error messages:

let match = version.match(...); if (!match) { echo(`You must pass a correctly formatted version; couldn't parse ${version}`); exit(1); }

ide · 2017-09-09T17:51:31Z

scripts/versiontemplates/ReactNativeVersion.java

+import java.util.Map;
+
+public class ReactNativeVersion {
+  public static final Map<String, Integer> VERSION = MapBuilder.of(


<String, @Nullable Object> since the prerelease value may be a String

ide · 2017-09-09T17:53:25Z

Libraries/Core/InitializeCore.js

@@ -116,6 +116,24 @@ if (!global.__fbDisableExceptionsManager) {
  ErrorUtils.setGlobalHandler(handleError);
 }

+const formatVersion = version =>
+  `${version.major}.${version.minor}.${version.patch}` +
+  (version.prerelease !== null ? `-rc.${version.prerelease}` : '');


Just concatenate the prerelease (instead of assuming -rc, see comments later in this review)

janicduplessis · 2017-09-10T05:57:28Z

@ide Updated

ide · 2017-09-11T07:02:20Z

Waiting for CI to pass.

ide · 2017-09-11T17:07:01Z

@janicduplessis it looks like the Jest setup script needs to be updated (see CircleCI output).

janicduplessis · 2017-09-11T19:59:46Z

Updated, tests now pass locally, let's wait for CI again. Ended up just providing an explicit mock for the InitializeCore module since jest automock feature does run the module which is when the error happened. This also makes sure we don't have to deal with the version check during tests.

chirag04 · 2017-09-11T20:27:29Z

Libraries/Core/InitializeCore.js

+
+const ReactNativeVersion = require('./ReactNativeVersion');
+const nativeVersion = require('NativeModules').PlatformConstants.reactNativeVersion;
+if (ReactNativeVersion.version.major !== nativeVersion.major ||


should we keep this behind the DEV flag?

@ide prefered not to, I don't really have a preference

The main thing is to keep dev and prod similar, particularly around edge cases, error handling, initialization, cleanup. If we move this to a __DEV__-only check, we should change this to a console.error call instead of throwing an error.

i would rather let it continue in prod than spectacularly failing at startup. idk

ide · 2017-09-11T21:50:44Z

CI failed again, probably need to tell Buck about the new Java file.

janicduplessis · 2017-09-11T22:15:56Z

Moved the file to the same package to avoid circular deps which buck doesn't support.

janicduplessis · 2017-09-12T07:56:07Z

Ok finally got CI passing :)

ide · 2017-09-12T10:08:29Z

This looks good to me.

@facebook-github-bot shipit

facebook-github-bot · 2017-09-12T10:08:55Z

@ide has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-09-12T23:48:42Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

ide · 2017-09-13T08:43:17Z

@hramos could you or someone else at fb look into why this failed to land? The OSS CI tests look good for it.

hramos · 2017-09-27T19:59:56Z

scripts/versiontemplates/ReactNativeVersion.js

+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ *
+ * @flow


This PR is not importing due to the following error:

Flow check failed: 1 Flow error and 0 Flow warnings detected! =====Flow error #0==== Error: scripts/versiontemplates/ReactNativeVersion.js line 15 col 11-11: Unexpected token {

use haste

facebook-github-bot · 2017-09-27T20:16:25Z

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Basic implementation of the proposal in #15271 Note that this should not affect facebook internally since they are not using OSS releases. Points to consider: - How strict should the version match be, right now I just match exact versions. - Wasn't able to use haste for ReactNativeVersion because I was getting duplicate module provider caused by the template file in scripts/versiontemplates. I tried adding the scripts folder to modulePathIgnorePatterns in package.json but that didn't help. - Redscreen vs. warning, I think warning is useless because if the app crashes you won't have time to see the warning. - Should the check and native modules be __DEV__ only? **Test plan** Tested that it works when version match and that it redscreens when versions don't before getting other errors on Android and iOS. Closes #15518 Differential Revision: D5813551 Pulled By: hramos fbshipit-source-id: 901757e25724b0f22bf39de172b56309d0dd5a95

janicduplessis requested review from javache and shergin as code owners August 16, 2017 20:29

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 16, 2017

anp mentioned this pull request Aug 25, 2017

Let users know if their Expo SDK version & sdkVersion in exp.json/app.json don't line up expo/expo#176

Closed

janicduplessis force-pushed the version-mismatch branch from 8d857d0 to 9ff1f80 Compare September 1, 2017 22:58

Validate that JS and Native code versions match for RN releases

04a9905

janicduplessis force-pushed the version-mismatch branch from 9ff1f80 to 04a9905 Compare September 1, 2017 23:00

janicduplessis force-pushed the version-mismatch branch from 31fb80e to 2e69a0e Compare September 9, 2017 06:52

Version map, check only major and minor

77522b2

janicduplessis force-pushed the version-mismatch branch from 2e69a0e to 77522b2 Compare September 9, 2017 09:52

ide suggested changes Sep 9, 2017

View reviewed changes

Make prerelease a string

00b9377

Merge branch 'master' into version-mismatch

54c7189

ide approved these changes Sep 11, 2017

View reviewed changes

Fix tests

67f10f0

chirag04 reviewed Sep 11, 2017

View reviewed changes

Move android version file to fix buck

1161c5a

janicduplessis added 3 commits September 11, 2017 18:55

Fix buck p2

71e94f1

Fix buck p3 :/

8a80f80

Fix package

cb97423

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Sep 12, 2017

facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Sep 12, 2017

hramos reviewed Sep 27, 2017

View reviewed changes

Rename template files to .template to avoid flow issues, can now also

b37f5e6

use haste

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Sep 27, 2017

facebook-github-bot closed this in 1af645b Sep 28, 2017

rozele mentioned this pull request Oct 4, 2017

Checking for a version match between JS and native #15271

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate that JS and Native code versions match for RN releases #15518

Validate that JS and Native code versions match for RN releases #15518

janicduplessis commented Aug 16, 2017 •

edited

Loading

janicduplessis commented Aug 16, 2017

pull-bot commented Aug 16, 2017 •

edited

Loading

ide commented Aug 17, 2017

javache commented Aug 25, 2017

ide commented Aug 25, 2017

javache commented Aug 29, 2017 •

edited

Loading

ide commented Aug 29, 2017

janicduplessis commented Sep 1, 2017

janicduplessis commented Sep 1, 2017

ide commented Sep 2, 2017

ide Sep 9, 2017

ide Sep 9, 2017

ide Sep 9, 2017

janicduplessis commented Sep 10, 2017

ide commented Sep 11, 2017

ide commented Sep 11, 2017

janicduplessis commented Sep 11, 2017 •

edited

Loading

chirag04 Sep 11, 2017 •

edited

Loading

janicduplessis Sep 11, 2017

ide Sep 11, 2017

chirag04 Sep 12, 2017

ide commented Sep 11, 2017

janicduplessis commented Sep 11, 2017

janicduplessis commented Sep 12, 2017

ide commented Sep 12, 2017

facebook-github-bot commented Sep 12, 2017

facebook-github-bot commented Sep 12, 2017

ide commented Sep 13, 2017

hramos Sep 27, 2017

facebook-github-bot commented Sep 27, 2017

Validate that JS and Native code versions match for RN releases #15518

Validate that JS and Native code versions match for RN releases #15518

Conversation

janicduplessis commented Aug 16, 2017 • edited Loading

janicduplessis commented Aug 16, 2017

pull-bot commented Aug 16, 2017 • edited Loading

ide commented Aug 17, 2017

javache commented Aug 25, 2017

ide commented Aug 25, 2017

javache commented Aug 29, 2017 • edited Loading

ide commented Aug 29, 2017

janicduplessis commented Sep 1, 2017

janicduplessis commented Sep 1, 2017

ide commented Sep 2, 2017

ide Sep 9, 2017

Choose a reason for hiding this comment

ide Sep 9, 2017

Choose a reason for hiding this comment

ide Sep 9, 2017

Choose a reason for hiding this comment

janicduplessis commented Sep 10, 2017

ide commented Sep 11, 2017

ide commented Sep 11, 2017

janicduplessis commented Sep 11, 2017 • edited Loading

chirag04 Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

janicduplessis Sep 11, 2017

Choose a reason for hiding this comment

ide Sep 11, 2017

Choose a reason for hiding this comment

chirag04 Sep 12, 2017

Choose a reason for hiding this comment

ide commented Sep 11, 2017

janicduplessis commented Sep 11, 2017

janicduplessis commented Sep 12, 2017

ide commented Sep 12, 2017

facebook-github-bot commented Sep 12, 2017

facebook-github-bot commented Sep 12, 2017

ide commented Sep 13, 2017

hramos Sep 27, 2017

Choose a reason for hiding this comment

facebook-github-bot commented Sep 27, 2017

janicduplessis commented Aug 16, 2017 •

edited

Loading

pull-bot commented Aug 16, 2017 •

edited

Loading

javache commented Aug 29, 2017 •

edited

Loading

janicduplessis commented Sep 11, 2017 •

edited

Loading

chirag04 Sep 11, 2017 •

edited

Loading