Skip to content

Conversation

CoryWritesCode
Copy link
Contributor

@CoryWritesCode CoryWritesCode commented Sep 27, 2021

As stated in #501 that fix isn't backwards compatible with previous versions of RN.

This should keep that compatibility viable while fixing the issue on the latest version of RN.

I created two apps working with different versions of RN to ensure the backwards compatibility.
You can view the code for them here

Fixes #508
Fixes #494

@CoryWritesCode CoryWritesCode marked this pull request as ready for review September 27, 2021 16:52
@CoryWritesCode
Copy link
Contributor Author

@slorber does this PR work for you?

@CoryWritesCode CoryWritesCode mentioned this pull request Sep 27, 2021
@mingtsay
Copy link

I have permission to merge and publish this package, but can't dedicate time to it.

Who has used the fork, and can confirm it works for both older versions of RN + 0.65?

Merging a change that only works for the new RN version would harm the community. What is the current RN support of current lib version? When was scrollTo introduced exactly?

Please make sure that it's working for you (on RN 0.65) but for others too, and invest your own time to convince me it's the case with a demo app or whatever, because I can't blindly merge this PR nor invest my own time atm.

@slorber You might want to see this PR. It will use scrollTo when the scrollResponderScrollTo is not available.
That will cover all legacy versions and support the versions from 0.65.

@jcgertig
Copy link

jcgertig commented Oct 4, 2021

@slorber @mingtsay please any movement on this would be great

Copy link

@bmatasar bmatasar left a comment

Choose a reason for hiding this comment

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

Originally there was a check if responder was defined and not null and it is gone now.
I would wrap the lines 276-282 and 287-293 in
if (responder) { ... }

@mingtsay
Copy link

I tested in 0.66 and it works. So no worry for the latest stable version. 🥂

CoryWritesCode and others added 2 commits October 27, 2021 15:01
Co-authored-by: Mihai <lazari_mihai@mail.ru>
Co-authored-by: Mihai <lazari_mihai@mail.ru>
@CoryWritesCode
Copy link
Contributor Author

@bmatasar I added @mlazari's checks into the PR. Thank you @mlazari!!

@scblason
Copy link

Any timeline in when this will be merged? Needed ASAP

@safaiyeh
Copy link

@slorber could we get some eyes on this?

@finalquest
Copy link

Hi guys, I don't wan't to push on this. But, as today November 1, android only supports api level 30.
To make that level work, we need to update RN to at least 0.65.0.
This issues will prevent a great number of devs to update.
Can we make a release with this fix ASAP.

Regards

@Noitham
Copy link

Noitham commented Nov 2, 2021

Hi guys, I don't wan't to push on this. But, as today November 1, android only supports api level 30. To make that level work, we need to update RN to at least 0.65.0. This issues will prevent a great number of devs to update. Can we make a release with this fix ASAP.

Regards

You can bumpcompileSdkVersion/targetSdkVersion without necessarily upgrading react-native. (currently on 63.3 with API level 30)

@ghost
Copy link

ghost commented Nov 3, 2021

So, when merge ?

@slorber slorber merged commit de9579b into APSL:master Nov 4, 2021
@slorber
Copy link
Collaborator

slorber commented Nov 4, 2021

Released in 9.5, thanks for the retro compatible change @CoryWritesCode and sorry for the delay


To everyone asking "please merge, I need this ASAP!!!", you are hurting the open-source community and actually delayed the merge of this PR:

  • you add noise, making it harder to review good feedbacks and reviews from real contributors
  • you make maintainers feel guilty and burn out, maintainers that you don't pay and owing to you nothing, leading to abandoned projects
  • it's easy to use a RN fork as a dependency (or patch-package), and if you NEED this PR, you can only blame yourself
  • this PR doesn't pass yarn test due to eslint issues => surprising for a PR that was considered ready to merge by everyone

@adias9
Copy link

adias9 commented Nov 4, 2021

Thanks @slorber for being responsive on this issue. I'm sorry many flooded this thread with noise. Really grateful for your work. ❤️ I think many people love this package and are going about expressing their love and the critical use for this package in an unfriendly way. If I may speak for them, we are super grateful that this package exists. I know it has helped in my team's dev time.

@CoryWritesCode CoryWritesCode deleted the corywritescode/backwards-capatable-fix-for-scroll-responder branch November 8, 2021 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: responder.scrollResponderScrollTo is not a function. (In 'responder.scrollResponderScrollTo library will crash on react native 0.65.0