Warning - 'getNode() deprecation fix'

[dilinade]

Platforms affected

Android & iOS

What does this PR do?

Fixes #672

What testing has been done on this change?

Tested features checklist

• Default setup (example)
• [] Carousels with and without momentum enabled (prop enableMomentum)
• [] Vertical carousels (prop vertical)
• Slide alignment (prop activeSlideAlignment)
• Autoplay (prop autoplay)
• Loop mode (prop loop)
• ScrollView/FlatList carousels (prop useScrollView)
• Callback methods
• ParallaxImage component
• Pagination component
• Layouts and custom interpolations

[yasaricli]

+1

[bd-arc]

@dilinade Thanks for the PR!

I'd be happy to merge this, but I fear this might break things for users who haven't migrated to the latest version of React Native...

Think you can make it backward-compatible?

[dilinade]

@bd-arc Hi, I have made some changes. Please review :)
Thank you.

[ifsnow]

@dilinade I think it's not efficient to check the version of React Native every time in _getWrappedRef(). Why not reuse it after checking it only once?

[bd-arc]

@dilinade Couple of notes:

1. I agree with @ifsnow: this should be done only once in the constructor.
2. It would be better to have a future-proof way of determining the version.
3. We probably want to err of the side of caution and make sure that everything is defined.
4. Please make sure to activate Eslint when working on the project ;-)

Putting it all together, here's my suggestion (untested). What do you guys think?

constructor (props) {
    super(props);

    // Everything else...

    this._shouldUseGetNode = this._useGetNode();
}

_useGetNode () {
    const package = require('../../../react-native/package.json');
    const version = package && package.version;
    if (!version) {
        return true;
    }
    const versionSplit = version.split('.');
    if (!versionSplit || !versionSplit.length) {
        return true;
    }
    const versionCode = versionSplit[0] * 1000000 + versionSplit[1] * 10000 + versionSplit[2] * 100;
    return versionCode < 620000; // RN 0.62
}

_getWrappedRef () {
    // https://github.com/facebook/react-native/issues/10635
    // https://stackoverflow.com/a/48786374/8412141
    return this._shouldUseGetNode ?
        this._carouselRef && this._carouselRef.getNode && this._carouselRef.getNode() :
        this._carouselRef;
}

[bd-arc]

@dilinade This is how I actually implemented it in version 4.x of the plugin I'm currently working on: 1b78117

[dilinade]

@bd-arc Got it and Cool :)

[timgarciaa]

i'm using this version 'react-native-snap-carousel@4.0.0-beta.2' but still got the error. Is this the same with this release?

[dotamir]

@dilinade Did you made the suggested changes? Is this PR ready to go forward?

[nickarora]

@dilinade @bd-arc -- can we get this merged and released?

[bd-arc]

Unfortunately the following basically breaks everything on Snack because it is undefined there: require('../../../react-native/package.json'). I assume Expo uses a different path and that it will break for all Expo users as well.

This is a quite a big issue and I'm scratching my head to find a way to make it work.

Requiring the package conditionally would be the obvious way to go, but unfortunately this is not supported by the React Native packager (see this thread for example).

Also tried with a try/catch, but encountered the same issue (conditional require not supported by metro bundle).

If anyone has an idea to address that, please let everyone know!

[r0b0t3d]

Could we check if _carouselRef contains setNativeProps function, so we do not need to check the version anymore.

if (this._carouselRef && this._carouselRef.setNativeProps) {
  return this._carouselRef;
}
return this._carouselRef && this._carouselRef.getNode && this._carouselRef.getNode();

[dotamir]

@bd-arc I guess Expo is using this to load the files: expo-asset

I'm not sure if we use this, that's gonna support RN too or not. Which I think it will because expo is supporting most of React Native API's these days.

Also take a look at this too: here

[vamshi9666]

+1

[bfaulk96]

Could we check if _carouselRef contains setNativeProps function, so we do not need to check the version anymore.

if (this._carouselRef && this._carouselRef.setNativeProps) {
  return this._carouselRef;
}
return this._carouselRef && this._carouselRef.getNode && this._carouselRef.getNode();

Well, this definitely seems to solve the problem on 0.62.2, can someone confirm that it still works on <0.60.0? Would be great to get a fix as simple as this into the beta

[bd-arc]

Just included @r0b0t3d's suggestion in the v4 beta branch and published version 4.0.0-beta.3 if you want to give it a try.

Note that I didn't test it on RN pre-0.6x.

If someone wants to jump in...

[bd-arc]

Unfortunately, the suggestion doesn't work on versions < 0.62.

Back to square one...

[bd-arc]

Fixed in version 3.9.1 with backward-compatible code.