Remove UUID dependency in favour of own key generation system

jeremybarbet / react-native-portalize

The simplest way to render anything on top of the rest.

MIT License

335 stars 22 forks source link

Remove UUID dependency in favour of own key generation system #11

Closed harveyappleton closed 4 years ago

harveyappleton commented 4 years ago

Hey @jeremybarbet, did some tweaking to generate our own solution to this.

Really happy if you'd rather go with UUID route.

But in summary, this:

keeps track of used keys,
retries if it hits a duplicate (up to 5 times, but thats such an edge case as it's so random it should be fine, just wanted to stop an endless loop of retries)
removes used keys on unmount

And doesn't have any external dependencies. Should fix #10 too

Let me know your thoughts or if my code could be better :-)

Thanks bossman

harveyappleton commented 4 years ago

Could you rebase against master? your changes already merged are also here.

I think I've rebased correctly! Please have a look :-) Next task is to work on utils/key gen :)

harveyappleton commented 4 years ago

@jeremybarbet I've created external hook: hooks/useGeneratedKey.ts The array of used keys doesn't need to be a ref because it's not stored within a React component that re-renders (just stored at the top of the hook file).

Testing seems to show all working! Attached is screenshot of it adding keys. It removes them successfully too.

No clashes yet (you can see how random the keys are!)

Let me know your thoughts :-) Screenshot 2020-08-18 at 13 05 53

harveyappleton commented 4 years ago

@jeremybarbet I've refactored as you've requested 👍

Only thing to note is there's no benefit to using Refs for the lets inside generate key, as they are all block-level scoped to the generateKey function and just used to generate the key temporarily. Is that ok?

Would love to know your thoughts on if my first dip into typescript went well!!

harveyappleton commented 4 years ago

I have only a few comments on the code style. Sorry I'm super picky on the coding guideline 🤓

No worries! Just committed all your suggestions, take a look and make sure youre happy :-)

jeremybarbet commented 4 years ago

Looks good now, I will release a new version and you can test it out! Thanks for your contribution!

Edit: Released in 1.0.7

edc123 commented 4 years ago

Hi @jeremybarbet - this seems to work great in all use cases I have in my app upon updating to 1.0.7 and removing the dep on the polyfill. I'm looking at that fresh issue (#12) but not sure what the scenario is, but perhaps we should check if that was under an earlier version. Great work @harveyappleton @jeremybarbet ! Thank you

benjaminreid commented 4 years ago

Yes, nice work chaps. I ran into a very similar issue with two modals on one screen and updating to 1.0.7 has fixed things for me.