Combine pull requests and bugfixes

[mrexodia]

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]

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]

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]

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

[webraptor]

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

[ynigoreyes]

@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]

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]

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]

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]

or @mrexodia :) you choose

[webraptor]

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]

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

[webraptor]

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]

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]

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 [email protected] 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]

Works great!

[jacquesdev]

@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]

@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]

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

[webraptor]

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]

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 [email protected] 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]

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]

thanks @webraptor.

Switching to your fork

[mrexodia]

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]

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]

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]

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]

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]

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 [email protected] 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]

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]

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.