search

nuclearpasta / react-native-drax

A drag-and-drop system for React Native
MIT License
554 stars 69 forks source link

Use remove() on effect instead of removeListener #125

Closed jmarnold closed 2 years ago

jmarnold commented 2 years ago

Silences warnings:

 WARN  EventEmitter.removeListener('change', ...): Method has been deprecated. Please instead use `remove()` on the subscription returned by `EventEmitter.addListener`.
lafiosca commented 2 years ago

Thank you, looks like this was deprecated in RN 0.64. My only concern with the PR would be to verify that this works in RN 0.62-0.63, which I don't have time to check at the moment. But if you have any availability to look into that before I do, I'd appreciate it.

vishal-bhil commented 2 years ago

@lafiosca please approve this PR

lafiosca commented 2 years ago

@vishal-bhil @jmarnold I need to know if it is a breaking change for RN 0.62-0.63 before I can merge it. This is so I can update the documentation if necessary. Unfortunately I have no availability to research this myself right now, so if either of you could volunteer, please try it and let me know your findings. In the meantime, I always encourage people to use patch-package or yarn to maintain local patches if they need them in production before they are available in a release.

tiprat commented 2 years ago

Hello! According to this thread https://stackoverflow.com/questions/69141707/react-native-warning-receiving-warning-from-the-console , the first answer : "There is an upstream change, where in react-native@0.65.0 they deprecated the old api in favour of a more terse name, so removeListener() becomes remove()." Considering this, instead of const listener = Dimensions.addEventListener('change', handler); return () => listener.remove(); Could we just do this to account for all RN versions ? const listener = Dimensions.addEventListener('change', handler); return () => listener.remove ? listener.remove() : Dimensions.removeEventListener('change', handler);

Thank you for all your work on this project!

lafiosca commented 2 years ago

Could we just do this to account for all RN versions ? const listener = Dimensions.addEventListener('change', handler); return () => listener.remove ? listener.remove() : Dimensions.removeEventListener('change', handler);

We could probably do something like this, although I suspect we'd need to wrap it with some casts to satisfy TypeScript.

Thank you for all your work on this project!

You're welcome, and I'm sorry I haven't had time to continue maintaining it to the same standards I used to.

ajstokar commented 2 years ago

HI @lafiosca ! We just updated our RN version and we're now running into this warning as well. Do you think you'll merge this PR soon?

lafiosca commented 2 years ago

@ajstokar I'll take a look now. Thank you.

lafiosca commented 2 years ago

It appears this is a breaking change and requires RN >= 0.65. However given that RN is currently on 0.68, that I am not including any other critical functionality in this release, and that Drax already required >= 0.62, I am going to make this a breaking change and bump the RN requirement from 0.62 to 0.65 for new versions of Drax. Since we are technically still in 0.x.x "pre-release" mode, this will be released as version 0.10.0. If somehow we later release new features/fixes that folks using specifically RN 0.62-0.64 need and there is enough demand, we can attempt to add the backward-compatibility at that time in a separate PR. I will add another note here when 0.10.0 ships in a bit.

lafiosca commented 2 years ago

@ajstokar I have released 0.10.2 which includes this change and bumps the minimum requirement to RN 0.65. Please give it a try and let me know if you run into any problems because I do not have a 0.65 project available to test with.

ajstokar commented 2 years ago

Amazing, thanks so much!

ajstokar commented 2 years ago

@lafiosca everything is working as expected without the warning! One minor thing I'm noticing is it looks like the animation when an item is dropped became more laggy than it was on the older version. Not sure if you're already aware of that or not. If not, I'm happy to add a new issue and document what I'm seeing.

lafiosca commented 2 years ago

@ajstokar I was not aware of it. I was unable to perform any testing with the new version. I don't think anything that I changed in this release should have affected animations though, so that's sort of troubling.

Looking here: https://github.com/nuclearpasta/react-native-drax/compare/cb8614ca08f391a11e945215f1082969bd6a1aa7...main

There are effectively two logic changes. The rest of the stuff is just types and dev dependencies.

  1. Change the removeListener to remove - shouldn't impact animations
  2. Allow overriding the DraxProvider's style - I don't think this should have an impact, but it's possible there's an oversight and it's causing additional re-rendering? If it was this, I think the impact would be worse than just animation lag on dropping items.

If you can produce an example which is not laggy with 0.9.3 but is laggy with 0.10.2, please do open an issue describing it. As noted before though, my availability is very limited on this project. Thank you!