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

Checking for a version match between JS and native #15271

Closed
ide opened this Issue Jul 29, 2017 · 15 comments

Comments

Projects
None yet
10 participants
@ide
Collaborator

ide commented Jul 29, 2017

Many of the issues people run into - with quite a wide variety of symptoms - are caused by a mismatch between the JS and native code. There isn't a single reason for this mismatch, ex: it could be caused by stale Xcode/Android build data, forgetting to rebuild the native app after upgrading RN in node_modules, Watchman/Metro caching bugs, bundle caching on the client, and so on. Some kind of simple version check might help people discover these bugs more quickly with a more accurate error message.

Proposal

We add three new files: ReactNativeVersion.java, RCTVersion.h, and ReactNativeVersion.js. These files are marked as @generated and are written only by the release scripts, which already modify package.json, the Podspec, and a Gradle file.

// ReactNativeVersion.java
// @generated by scripts/bump-oss-version.js
public class ReactNativeVersion {
  public static final String VERSION = "0.47.0-rc.1";
  public static final int MAJOR = 0;
  public static final int MINOR = 47;
  public static final int PATCH = 0;
  public static final int PRERELEASE = "rc.1";
}
// RCTVersion.h
// @generated by scripts/bump-oss-version.js
#define REACT_NATIVE_VERSION @"0.47.0-rc.1"
#define REACT_NATIVE_VERSION_MAJOR 0
#define REACT_NATIVE_VERSION_MINOR 47
#define REACT_NATIVE_VERSION_PATCH 0
#define REACT_NATIVE_VERSION_PRERELEASE @"rc.1"
// ReactNativeVersion.js
// @generated by scripts/bump-oss-version.js
exports.version = '0.47.0-rc.1';
exports.major = 0;
exports.minor = 47;
exports.patch = 0;
exports.prerelease = 'rc.1'

These files are checked into the repo for simplicity. In addition to these autogenerated files, there would be small native modules that export these versions from native to JS (using constantsToExport, it should be fast since there's not much real work being done). JS would then compare ReactNativeVersion.version to NativeModules.Version.version and call console.error if they don't match (for some definition of match -- maybe it's OK if 0.46.1 JS runs on 0.46.0 native code, don't want this warning to be annoying).

@dlowder-salesforce

This comment has been minimized.

Show comment
Hide comment
@dlowder-salesforce

dlowder-salesforce Jul 30, 2017

Collaborator

I like this idea a lot! Maybe version should be an object instead of a string... what about

// ReactNativeVersion.js
// @generated by scripts/bump-oss-version.js
exports.version = {
  'major' : '0.47',
  'minor' : '0',
  'build' : ''
};

Maybe console.error should only happen if there is a mismatch in the major string. Build could be something used by app developers who might be making customizations in RN code.

Collaborator

dlowder-salesforce commented Jul 30, 2017

I like this idea a lot! Maybe version should be an object instead of a string... what about

// ReactNativeVersion.js
// @generated by scripts/bump-oss-version.js
exports.version = {
  'major' : '0.47',
  'minor' : '0',
  'build' : ''
};

Maybe console.error should only happen if there is a mismatch in the major string. Build could be something used by app developers who might be making customizations in RN code.

@jpshelley

This comment has been minimized.

Show comment
Hide comment
@jpshelley

jpshelley Jul 31, 2017

Contributor

fwiw I've seen issues pop up when Native/JS libs are mismatched across minor versions as well @dlowder-salesforce (I think loading images had broke for me between 0.43.0 and 0.43.2 or something If I remember correctly). I like the object idea but would want minor version checked in the diff as well though.

Contributor

jpshelley commented Jul 31, 2017

fwiw I've seen issues pop up when Native/JS libs are mismatched across minor versions as well @dlowder-salesforce (I think loading images had broke for me between 0.43.0 and 0.43.2 or something If I remember correctly). I like the object idea but would want minor version checked in the diff as well though.

@SimplGy

This comment has been minimized.

Show comment
Hide comment
@SimplGy

SimplGy Jul 31, 2017

This would be a huge help. What a great way to remove a whole class of confusing errors and replace with a simple, accurate message.

SimplGy commented Jul 31, 2017

This would be a huge help. What a great way to remove a whole class of confusing errors and replace with a simple, accurate message.

@ide ide added the Help Wanted :octocat: label Aug 16, 2017

facebook-github-bot added a commit that referenced this issue Sep 28, 2017

Validate that JS and Native code versions match for RN releases
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

ide added a commit that referenced this issue Sep 28, 2017

Validate that JS and Native code versions match for RN releases
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
@rozele

This comment has been minimized.

Show comment
Hide comment
@rozele

rozele Oct 4, 2017

Collaborator

Do you guys have any proposals for how platform extensions play along with this? Unfortunately this proposal only covers iOS and Android (or any platform in this repo). For react-native-windows, we may be using JavaScript from this repo but native code from a separate repo, and this is yet another thing we have to keep in sync. It would be nice if this had hooks that accommodated for this (cc @axemclion).

I like that the basic implementation in #15518 only checks against the major/minor, because we tend to keep these in sync with react-native, but it would become very problematic if this check were to also include the build version (these do not have to be in sync). Also, is there a plan for updating this when we eventually start shipping major versions of react-native or is it "we'll cross that bridge when we get there"?

Collaborator

rozele commented Oct 4, 2017

Do you guys have any proposals for how platform extensions play along with this? Unfortunately this proposal only covers iOS and Android (or any platform in this repo). For react-native-windows, we may be using JavaScript from this repo but native code from a separate repo, and this is yet another thing we have to keep in sync. It would be nice if this had hooks that accommodated for this (cc @axemclion).

I like that the basic implementation in #15518 only checks against the major/minor, because we tend to keep these in sync with react-native, but it would become very problematic if this check were to also include the build version (these do not have to be in sync). Also, is there a plan for updating this when we eventually start shipping major versions of react-native or is it "we'll cross that bridge when we get there"?

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Oct 4, 2017

Collaborator

If I understand correctly, the version check requires the JS to line up with the native code the same way the rest of React Native does. For react-native-windows, you'd do two things: make the native PlatformConstants module export a reactNativeVersion field, and override the ReactNativeVersion module to export the same value.

I think the approach above wouldn't be affected if RN were to check the build version since react-native-windows would have control over the version number it defines in both native and JS -- the only thing react-native-windows doesn't control is the checking mechanism in InitializeCore.js. We could also add a new module called VersionCheck.js that InitializeCore.js uses, and react-native-windows could override the former.

I haven't heard of a plan for major versions of RN but I don't think it materially changes things. There is a question of whether the native code from 1.1.0 would support JS code from 1.0.0 but we already have that same question today, just shifted over by one point (does native 0.1.1 support JS 0.1.0?). The current answer is "probably yes" hence the heuristic used in the current version check but no guarantees -- best to line up the numbers precisely.

Collaborator

ide commented Oct 4, 2017

If I understand correctly, the version check requires the JS to line up with the native code the same way the rest of React Native does. For react-native-windows, you'd do two things: make the native PlatformConstants module export a reactNativeVersion field, and override the ReactNativeVersion module to export the same value.

I think the approach above wouldn't be affected if RN were to check the build version since react-native-windows would have control over the version number it defines in both native and JS -- the only thing react-native-windows doesn't control is the checking mechanism in InitializeCore.js. We could also add a new module called VersionCheck.js that InitializeCore.js uses, and react-native-windows could override the former.

I haven't heard of a plan for major versions of RN but I don't think it materially changes things. There is a question of whether the native code from 1.1.0 would support JS code from 1.0.0 but we already have that same question today, just shifted over by one point (does native 0.1.1 support JS 0.1.0?). The current answer is "probably yes" hence the heuristic used in the current version check but no guarantees -- best to line up the numbers precisely.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Oct 4, 2017

Collaborator

This version check is also out with 0.49 so I'm going to close this issue (for task-tracking, not to close off future conversation).

Collaborator

ide commented Oct 4, 2017

This version check is also out with 0.49 so I'm going to close this issue (for task-tracking, not to close off future conversation).

@ide ide closed this Oct 4, 2017

@rozele

This comment has been minimized.

Show comment
Hide comment
@rozele

rozele Oct 5, 2017

Collaborator

@ide I didn't word it very well. The UWP platform extension is composed of a native implementation, the core React Native JavaScript, and the platform specific JavaScript overrides. The current approach assumes a 1-to-1 mapping between native and core JS libraries. If I override the ReactNativeVersion module, I have no way of asserting that the core React Native JavaScript has the correct version, only that my platform specific JavaScript matches.

Collaborator

rozele commented Oct 5, 2017

@ide I didn't word it very well. The UWP platform extension is composed of a native implementation, the core React Native JavaScript, and the platform specific JavaScript overrides. The current approach assumes a 1-to-1 mapping between native and core JS libraries. If I override the ReactNativeVersion module, I have no way of asserting that the core React Native JavaScript has the correct version, only that my platform specific JavaScript matches.

@dantman

This comment has been minimized.

Show comment
Hide comment
@dantman

dantman Oct 5, 2017

Contributor

I presume it's possible for platform extensions to extend ReactNativeVersion.js rather than straight override it (ie: define a ReactNativeVersion.yourPlatform.js that requires the non-suffixed ReactNativeVersion.js provided by react-native and exports a modified version).

How about adding some semi-official semi-support for platform extensions. Not fully supporting them, but acknowledging they are part of the react-native ecosystem and providing a reasonable integration for them into the version handling.

For that in the version context, I think:

ReactNativeVersion.js should export in this format { major: number, minor: number, patch: number, prerelease: null, platform?: { major: number, minor: number, patch: number, prerelease: null } }.

On normal react-native both require('ReactNativeVersion').version.platform and NativeModules.PlatformConstants.reactNativeVersion.platform should be null. InitializeCore.js
should have a second section comparing the platform version if it's not null and making sure that one is null if the other is null.

Platform extensions will extend ReactNativeVersion.js and return the same major/minor/patch but export their own platform version.

In NativeModules.PlatformConstants.reactNativeVersion, the platform extension will define the major/minor/patch of the react-native version that their native code is coded against (ie: written to behave like) and under platform define their own version.

Then platform extensions have versions properly handled. The react-native version validates that the platform extension native code is coded to behave like the version the react-native js code is expecting. And the platform version validates that the platform js matches the version of platform native code running.

If tacking on a nested structure doesn't feel right, we could go straight into making it slightly more official and have a separate ReactNativeVersion.js and ReactNativePlatformVersion.js (react would return null from this) or ReactNativeVersion.js could export version and platform as separate exports.

Contributor

dantman commented Oct 5, 2017

I presume it's possible for platform extensions to extend ReactNativeVersion.js rather than straight override it (ie: define a ReactNativeVersion.yourPlatform.js that requires the non-suffixed ReactNativeVersion.js provided by react-native and exports a modified version).

How about adding some semi-official semi-support for platform extensions. Not fully supporting them, but acknowledging they are part of the react-native ecosystem and providing a reasonable integration for them into the version handling.

For that in the version context, I think:

ReactNativeVersion.js should export in this format { major: number, minor: number, patch: number, prerelease: null, platform?: { major: number, minor: number, patch: number, prerelease: null } }.

On normal react-native both require('ReactNativeVersion').version.platform and NativeModules.PlatformConstants.reactNativeVersion.platform should be null. InitializeCore.js
should have a second section comparing the platform version if it's not null and making sure that one is null if the other is null.

Platform extensions will extend ReactNativeVersion.js and return the same major/minor/patch but export their own platform version.

In NativeModules.PlatformConstants.reactNativeVersion, the platform extension will define the major/minor/patch of the react-native version that their native code is coded against (ie: written to behave like) and under platform define their own version.

Then platform extensions have versions properly handled. The react-native version validates that the platform extension native code is coded to behave like the version the react-native js code is expecting. And the platform version validates that the platform js matches the version of platform native code running.

If tacking on a nested structure doesn't feel right, we could go straight into making it slightly more official and have a separate ReactNativeVersion.js and ReactNativePlatformVersion.js (react would return null from this) or ReactNativeVersion.js could export version and platform as separate exports.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Oct 5, 2017

Collaborator

@rozele oh ok, I answered your first comment from the angle of "how can we not break react-native-windows?" rather than "how can react-native-windows do version checking?".

A simple and flexible approach that I suspect wouldn't need many upstream changes over time would be to add a new module called ReactNativeVersionCheck.js whose default implementation is:

const {PlatformConstants} =  require('NativeModules');
const ReactNativeVersion = require('ReactNativeVersion);

exports.checkVersions = function checkVersions(): void {
  if (ReactNativeVersion ≠ PlatformConstants.reactNativeVersion) {
    throw new Error(...);
  }
}

InitializeCore.js would run:

ReactNativeVersionCheck.checkVersions();

This provides the same behavior as what's in 0.49 today. react-native-windows would provide a platform-specific override of ReactNativeVersionCheck.js where it could run whatever it wanted. The reason I like this is it potentially lets us do all three version checks: (RNW C#, RN Core JS), (RNW C#, RNW JS), and (RN Core JS, RNW JS) and RN Core would only have to try to keep ReactNativeVersionCheck.checkVersions() stable, for which we could add some unit tests.

Collaborator

ide commented Oct 5, 2017

@rozele oh ok, I answered your first comment from the angle of "how can we not break react-native-windows?" rather than "how can react-native-windows do version checking?".

A simple and flexible approach that I suspect wouldn't need many upstream changes over time would be to add a new module called ReactNativeVersionCheck.js whose default implementation is:

const {PlatformConstants} =  require('NativeModules');
const ReactNativeVersion = require('ReactNativeVersion);

exports.checkVersions = function checkVersions(): void {
  if (ReactNativeVersion ≠ PlatformConstants.reactNativeVersion) {
    throw new Error(...);
  }
}

InitializeCore.js would run:

ReactNativeVersionCheck.checkVersions();

This provides the same behavior as what's in 0.49 today. react-native-windows would provide a platform-specific override of ReactNativeVersionCheck.js where it could run whatever it wanted. The reason I like this is it potentially lets us do all three version checks: (RNW C#, RN Core JS), (RNW C#, RNW JS), and (RN Core JS, RNW JS) and RN Core would only have to try to keep ReactNativeVersionCheck.checkVersions() stable, for which we could add some unit tests.

@rozele

This comment has been minimized.

Show comment
Hide comment
@rozele

rozele Oct 5, 2017

Collaborator

Thanks @ide, I think that's a sound solution. Is this something you think we should patch into 0.49 or wait for 0.50?

One other question: I'm sure it's rare, but sometimes a version of react-native JS is compatible with a previous version of react-native binaries. Or even if it's not fully compatible, maybe the only part that is no longer compatible is no longer being used in my app. The scenario I'm watching out for is a CodePush user who acknowledges the risks of updating react-native JS only, but wants to get a new feature out that's available in the latest react-native JS, and has fully tested compatibility with the native modules they are using. It seems that this version check "feature" is preventing users who may want to do this.

Collaborator

rozele commented Oct 5, 2017

Thanks @ide, I think that's a sound solution. Is this something you think we should patch into 0.49 or wait for 0.50?

One other question: I'm sure it's rare, but sometimes a version of react-native JS is compatible with a previous version of react-native binaries. Or even if it's not fully compatible, maybe the only part that is no longer compatible is no longer being used in my app. The scenario I'm watching out for is a CodePush user who acknowledges the risks of updating react-native JS only, but wants to get a new feature out that's available in the latest react-native JS, and has fully tested compatibility with the native modules they are using. It seems that this version check "feature" is preventing users who may want to do this.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Oct 5, 2017

Collaborator

I think we'd make the change in 0.50 since react-native-windows can still implement PlatformConstants.reactNativeVersion in 0.49 and there's no regression compared to 0.48 and before.

For the CodePush scenario -- yes, this blocks that though I suspect it is uncommon and it's better to think of React Native as a single unit that's separate from app code, rather than drawing the boundaries of that unit around the RN JS + app code separately from the RN native code.

Regardless it'd be nice if we could provide an escape hatch somehow (e.g. by registering some custom hooks with RN before it fully initializes). For now we're erring on the side of soundness since mismatched JS and native has caused several non-obvious issues for people that could have been fixed quickly.

Collaborator

ide commented Oct 5, 2017

I think we'd make the change in 0.50 since react-native-windows can still implement PlatformConstants.reactNativeVersion in 0.49 and there's no regression compared to 0.48 and before.

For the CodePush scenario -- yes, this blocks that though I suspect it is uncommon and it's better to think of React Native as a single unit that's separate from app code, rather than drawing the boundaries of that unit around the RN JS + app code separately from the RN native code.

Regardless it'd be nice if we could provide an escape hatch somehow (e.g. by registering some custom hooks with RN before it fully initializes). For now we're erring on the side of soundness since mismatched JS and native has caused several non-obvious issues for people that could have been fixed quickly.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Oct 9, 2017

Contributor

Adding CodePush folks to this thread - @max-mironov and @sergey-akhalkov. Max/Sergey, could you also add Patrick and others who should be aware of this ?

Contributor

axemclion commented Oct 9, 2017

Adding CodePush folks to this thread - @max-mironov and @sergey-akhalkov. Max/Sergey, could you also add Patrick and others who should be aware of this ?

@max-mironov

This comment has been minimized.

Show comment
Hide comment
@max-mironov

max-mironov Oct 9, 2017

Thanks guys! Adding @pniko , @buptkang, @ruslan-bikkinin for awareness. We'll also discuss this internally how to align it with CodePush well.

max-mironov commented Oct 9, 2017

Thanks guys! Adding @pniko , @buptkang, @ruslan-bikkinin for awareness. We'll also discuss this internally how to align it with CodePush well.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Oct 16, 2017

Collaborator

Moved the version check into its own module (react-native-windows can override it and do whatever it wants in there) + added unit tests to make the interface more reliable in this PR: #16403

Collaborator

ide commented Oct 16, 2017

Moved the version check into its own module (react-native-windows can override it and do whatever it wants in there) + added unit tests to make the interface more reliable in this PR: #16403

@allenhsu

This comment has been minimized.

Show comment
Hide comment
@allenhsu

allenhsu Jan 25, 2018

I noticed RCTVersion.h is not a public header, so I cannot import it in my project and check versions on my own. Why not making it public (adding to Copy Headers phase of React project and podspec accordingly)?

allenhsu commented Jan 25, 2018

I noticed RCTVersion.h is not a public header, so I cannot import it in my project and check versions on my own. Why not making it public (adding to Copy Headers phase of React project and podspec accordingly)?

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