search

alexbrillant / react-native-deck-swiper

tinder like react-native deck swiper
ISC License
1.55k stars 461 forks source link

Combine pull requests and bugfixes #311

Open mrexodia opened 4 years ago

mrexodia commented 4 years ago

Combination of various pull requests and relevant commits from the various forks. Also includes an updated example that is more helpful.

Closes #309

Closes #285

Closes #268

Closes #252

Closes #245

Closes #247

To use this in your project:

npm install --save mrexodia/react-native-deck-swiper#combine-prs
ynigoreyes commented 4 years ago

Hi! I just submitted a PR as well. Do the main contributors still maintain this repo? I would really like to continue using it...

mrexodia commented 4 years ago

I don’t think so, but you are welcome to take my changes and fork. I’m using my fork in an app right now and everything works great (although there are still some rendering bugs with infinite cards, but I don’t think those are new)

ynigoreyes commented 4 years ago

That sucks to hear (the maintenance part, great initiative on your end haha) I’ll give it a look

webraptor commented 4 years ago

Tried to get in touch recently with @alexbrillant to help out with maintaining / taking over but no reply :(

ynigoreyes commented 4 years ago

@webraptor well I’m looking to take this code to production and but I also don’t want to be rude and use it without contributing back. If you need help maintaining this, I don’t mind at all. I can imagine myself using this for a while. It’s a very helpful piece of code

mrexodia commented 4 years ago

If anyone would like access to update this branch on my repo (and thus this pull request), let me know and I'll give you access to my fork!

jacquesdev commented 4 years ago

I'm also happy to help. I am going to try and track him down to switch ownership. Otherwise, we must just agree amongst ourselves who is going to fork it - and then stick with that. Obviously easier if it gets officially handed over

jacquesdev commented 4 years ago

I've tried getting in contact with @alexbrillant, on many different platforms, but he has not come back. Going to suggest that people start to @webraptor's fork instead?

jacquesdev commented 4 years ago

or @mrexodia :) you choose

webraptor commented 4 years ago

I've tried getting in contact with @alexbrillant, on many different platforms, but he has not come back. Going to suggest that people start to @webraptor's fork instead?

Been there, done that. I think I even commented on an open PRs on which he was actively working on ...

jacquesdev commented 4 years ago

Ok thanks @webraptor, switching to yours. I suggest adding a new issue, suggesting people move over

webraptor commented 4 years ago

Ok thanks @webraptor, switching to yours. I suggest adding a new issue, suggesting people move over

Yes, but even so I won't be able to push to npm the same package. That's why @alexbrillant response is to be pursued...

jacquesdev commented 4 years ago

I'd suggest that you create a new npm package for now called react-native-deck-swiper2? or react-native-deck-swiper-pro :)

Can you please confirm whether your fork already has all this work merged in that @mrexodia has in their fork?

mrexodia commented 4 years ago

I can open a pull request if necessary, but for my purposes my fork is already sufficient and works great!

On Thu, 23 Jul 2020 at 11:38, Jacques de Villiers notifications@github.com wrote:

I'd suggest that you create a new npm package for now called react-native-deck-swiper2? or react-native-deck-swiper-pro :)

Can you please confirm whether your fork already has all this work merged in that @mrexodia https://github.com/mrexodia has in their fork?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/alexbrillant/react-native-deck-swiper/pull/311#issuecomment-662912624, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASYFGKAPSUFOGTPDBGZDBTR5AAJ5ANCNFSM4N5PHI7A .

rlee80 commented 4 years ago

Works great!

jacquesdev commented 4 years ago

@webraptor - will you set that up?

I need to be able to use this in react-native 0.62, and currently it's preventing me from upgrading

webraptor commented 4 years ago

@webraptor - will you set that up?

I need to be able to use this in react-native 0.62, and currently it's preventing me from upgrading

Yes, I'll try to do it asap and will let you guys know. I''ll contact npm to see if there's any possibility to preserve the package name. It's been over 2 months now since I've tried getting in touch with @alexbrillant and no response.

ekzotech commented 4 years ago

Thank you for this update! I hope there will be working fork. Tell me if I can help. Good luck!

webraptor commented 4 years ago

I've updated the fork. I already updated the readme with info for issues and a new To Do board.

I'll try to check existing PRs by the end of week and let their openers know that they can change their merge destination. The current priority is to update the example app and upgrade the package so it works with latest RN version so further bugfixes and features can be done.

mrexodia commented 4 years ago

Just so you know, many of the PRs are utter garbage. If I recall correctly I already updated the example in this PR as well.

On Wed, 29 Jul 2020 at 08:43, Bogdan Pop notifications@github.com wrote:

I've updated the fork https://github.com/webraptor/react-native-deck-swiper. I already updated the readme with info for issues and a new To Do https://github.com/webraptor/react-native-deck-swiper/projects/1 board.

I'll try to check existing PRs by the end of week and let their openers know that they can change their merge destination. The current priority is to update the example app and upgrade the package so it works with latest RN version so further bugfixes and features can be done.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/alexbrillant/react-native-deck-swiper/pull/311#issuecomment-665466586, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASYFGO33YUAUNOILEU4E7DR57AIFANCNFSM4N5PHI7A .

webraptor commented 4 years ago

Just so you know, many of the PRs are utter garbage. If I recall correctly I already updated the example in this PR as well. …

Thanks for the heads up. I plan to do code reviews for submitted PRs and some basic testing using an improvement example app to make sure we keep a high quality for the package.

We kinda did the same up until last year, so this might be a reason why Alex is unresponsive if he hasn't got the time.

jacquesdev commented 4 years ago

thanks @webraptor.

Switching to your fork

mrexodia commented 4 years ago

I would like to point out that @webraptor has not even merged my changes to make things work in their fork. I understand everybody is busy and just doing this in their free time, but right now I don't see a benefit to switch. This PR works fine on the latest(ish) Expo and appears to fix pretty much all blocking issues.

jacquesdev commented 4 years ago

I'm sure @webraptor will get to it soon. The main issue here is that as things are I cannot upgrade to rn0.62 due to issues in the plugin, and I'm sure it will get done shortly

webraptor commented 4 years ago

I would like to point out that @webraptor has not even merged my changes to make things work in their fork. I understand everybody is busy and just doing this in their free time, but right now I don't see a benefit to switch. This PR works fine on the latest(ish) Expo and appears to fix pretty much all blocking issues.

Hey @mrexodia. There's only one PR on the new fork. Also, within this PR the code in the componentDidUpdate method is not even checking the types when comparing the card arrays. The code would need some refactoring before being merged.

mrexodia commented 4 years ago

Thing is that currently the component is not working and with the fixes it is working. I'm no javascript/react expert, but from what I understood you only need to know if the references are different to see whether the property is updated or not, so both == and === will do the job just fine.

Anyway I changed it to match the best practice/code around it but it really doesn't make a difference 🙂

webraptor commented 4 years ago

Thing is that currently the component is not working and with the fixes it is working. I'm no javascript/react expert, but from what I understood you only need to know if the references are different to see whether the property is updated or not, so both == and === will do the job just fine.

Anyway I changed it to match the best practice/code around it but it really doesn't make a difference 🙂

Yes but the idea is that by simply comparing arrays like this you will get a lot of false positives, triggering the deck to re-render when it shouldn't.

The code was similar to what you're proposing years ago and with these changes we're going back to an unresponsive deck and we're trying to compensate on the effects rather than fix the cause.

And one of the reasons you need the componentDidUpdate fix is that the shouldComponentUpdate is no longer working properly because of the shallow comparison between the arrays.

mrexodia commented 4 years ago

Why would you get false positives? You set the property on the component from some state or another property and the reference won’t magically change if you don’t change the state right?

The only way you could get a false positive is if you somehow keep reusing the same array, clear that and add new elements but from what I understand that’s not the way you’re supposed to work anyway. Additionally a deep comparison has similar issues (which I encountered) in that when your server returns the same data the deck view will not update when it’s actually supposed to.

I can add some logs to check, but when I checked I didn’t get any unnecessary rerendering.

On Wed, 12 Aug 2020 at 08:37, Bogdan Pop notifications@github.com wrote:

Thing is that currently the component is not working and with the fixes it is working. I'm no javascript/react expert, but from what I understood you only need to know if the references are different to see whether the property is updated or not, so both == and === will do the job just fine.

Anyway I changed it to match the best practice/code around it but it really doesn't make a difference 🙂

Yes but the idea is that by simply comparing arrays like this you will get a lot of false positives, triggering the deck to re-render when it shouldn't.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/alexbrillant/react-native-deck-swiper/pull/311#issuecomment-672640251, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASYFGIXVQ7EVHIJGTXUPC3SAI2DLANCNFSM4N5PHI7A .

webraptor commented 4 years ago

Why would you get false positives? You set the property on the component from some state or another property and the reference won’t magically change if you don’t change the state right? The only way you could get a false positive is if you somehow keep reusing the same array, clear that and add new elements but from what I understand that’s not the way you’re supposed to work anyway. Additionally a deep comparison has similar issues (which I encountered) in that when your server returns the same data the deck view will not update when it’s actually supposed to. I can add some logs to check, but when I checked I didn’t get any unnecessary rerendering. … On Wed, 12 Aug 2020 at 08:37, Bogdan Pop @.**> wrote: Thing is that currently the component is not working and with the fixes it is working. I'm no javascript/react expert, but from what I understood you only need to know if the references* are different to see whether the property is updated or not, so both == and === will do the job just fine. Anyway I changed it to match the best practice/code around it but it really doesn't make a difference 🙂 Yes but the idea is that by simply comparing arrays like this you will get a lot of false positives, triggering the deck to re-render when it shouldn't. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#311 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASYFGIXVQ7EVHIJGTXUPC3SAI2DLANCNFSM4N5PHI7A .

That's actually exactly what happens. You could have only a few attributes of some of the cards change, or new cards may get added to the list (which is probably with high incidence as a result of likely paginating results etc). And of top of that the comparison is for arrays containing objects.

Just try running the following code in a console

var jangoFett = {
    occupation: "Bounty Hunter",
    genetics: "superb"
};

var bobaFett = {
    occupation: "Bounty Hunter",
    genetics: "superb"
};

// Outputs: false
console.log(bobaFett === jangoFett);
mrexodia commented 4 years ago

I'm well aware of how Javascript comparison works, thanks 🙂 I use the !== intentionally and not a deep compare.

For me the card stack is an immutable collection of cards and when you reach the end you will get a new stack. If you were to do a deep compare instead you will reset the whole stack if you update a single card (including the current index etc) so that wouldn't work.

Likely the pagination was the original reason that everything was hacked together the way it was. You were supposed to set the cards property a single time and then modify that collection to do your updates, but it causes different issues because you couldn't force an update without resorting to hacks so I'm not sure how to solve this properly.

webraptor commented 4 years ago

For me

The idea is that you can't think just about your single usage of the package. There's hundred+ projects using this and you have to think of the bigger picture. The deck should update when cards change, and since the renderCard is external anyone can control whether they want to re-render the card on top when this happens or not.

mrexodia commented 4 years ago

No need to get hostile. If you show me an example that previously actually worked correctly and no longer works with these changes I'm happy to think of a solution together. Saying 'Oh everybody is using this package with a bunch of hacks on top to make it work for their individual use case, so we cannot change a broken design' is not really conducive to improving the library in my opinion. If there is a breaking change you can change the major version and provide the relevant information to upgrade if necessary.

For me

Yes, from my perspective. I just read a bunch of things and concluded that props should be immutable and are passed to the component as an argument. The resources I found:

For the pagination you mentioned I would expect you pass a new props object together with a cardIndex to keep displaying the same card on the top if necessary, but I don't know exactly how it's supposed to work.

Perhaps in shouldComponentUpdate consider the property changed when either the object reference changed or the array contents change:

const propsChanged = (
    props.cards !== nextProps.cards ||
    !isEqual(props.cards, nextProps.cards) ||
    props.cardIndex !== nextProps.cardIndex
    )

And then in componentDidUpdate only rebuild the state when the reference changed? This way if the user decides to violate what I understand to be an invariant of props things will still work, with the only cost being that you have to do a deep comparison on the whole cards array.

jacquesdev commented 4 years ago

Hey guys, I know that things can get lost in translation in PR comments. I don't think anyone is intending to make things hostile. I think it's a great discussion to have, to find the best solution, so let's keep at it, but don't take anything personal.

Maybe it will help to first decide how it should work in an ideal world, and then to figure out how it is working at the moment. Then I would suggest that we add a param with a default value of true where it continues to use the existing functionality (to make it backward compatible), and then to potentially also allow the 'better' implementation.

Then we could in a future version deprecate that and change the default implementation.

I can't really talk about the details of this pr right now, not at my computer, but in principle I think we can find a solution that works for everyone

mrexodia commented 4 years ago

Apologies, I misread your comment :) Perhaps when you have some time go over the code properly, I spent a lot of time on it and I think we can reach a solution that will work without a backwards compatibility flag or something like that.

On Wed, 12 Aug 2020 at 12:07, Jacques de Villiers notifications@github.com wrote:

Hey guys, I know that things can get lost in translation in PR comments. I don't think anyone is intending to make things hostile. I think it's a great discussion to have, to find the best solution, so let's keep at it, but don't take anything personal.

Maybe it will help to first decide how it should work in an ideal world, and then to figure out how it is working at the moment. Then I would suggest that we add a param with a default value of true where it continues to use the existing functionality (to make it backward compatible), and then to potentially also allow the 'better' implementation.

Then we could in a future version deprecate that and change the default implementation.

I can't really talk about the details of this pr right now, not at my computer, but in principle I think we can find a solution that works for everyone

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/alexbrillant/react-native-deck-swiper/pull/311#issuecomment-672780864, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASYFGIHQHD2HJG5RSDGDBTSAJSUNANCNFSM4N5PHI7A .