search

jeremybarbet / react-native-portalize

The simplest way to render anything on top of the rest.
MIT License
335 stars 22 forks source link

Add random number to key to avoid clashes on rendering #9

Closed harveyappleton closed 4 years ago

harveyappleton commented 4 years ago

Avoids issues where multiple portals on one screen can have same key (Date.now() is not sufficient on its own if rendering is fast enough).

Could look at using something like uuid instead of a random number to guarantee more uniqueness? 👍

jeremybarbet commented 4 years ago

@harveyappleton Welcome to the open-source contribution world!

I totally agree, instead of trying to add a bit more of uniqueness each time, probably better to go directly with uuid so we shouldn't have any other problem moving forward.

Thanks for your PR!

harveyappleton commented 4 years ago

Sounds good, happy to investigate this and do a new pull req with uuid integrated!

harveyappleton commented 4 years ago

@jeremybarbet - rather than depending on another library (uuid), what about using a simple unique function? eg - https://gist.github.com/gordonbrander/2230317#gistcomment-3404537

jeremybarbet commented 4 years ago

@harveyappleton Sure sounds good, let's try that!

harveyappleton commented 4 years ago

@jeremybarbet I ended up using the UUID library because the gist i linked to there sadly didnt work as it also relied on Date.now() for randomness. Preliminary tests seem to show 5 portals on 1 react navigation screen working well!

Appreciate any feedback as TS / OpenSource contribution goodness is new to me, think I've done it correctly but appreciate any comments :-) thanks guys

jeremybarbet commented 4 years ago

Thank you! I will release a new version so you can try it out :)

Edit: 1.0.6 now available

edc123 commented 4 years ago

Oh I see it is time for me to magically reappear!

The github gist - as you mentioned - is just an extra letter of the alphabet away from "Date.now() + a random number", but I think this approach seemed "good enough".

Upon testing 1.0.6 in my app, I immediately got the following error: https://github.com/uuidjs/uuid#getrandomvalues-not-supported

When I follow these instructions to install the polyfill in my project - things work great and I don't seem to have any more issues with multiple instances. I'm going to run with it on my project.

Throwing it back to you peeps! (Through a new github issue... sorry)

jeremybarbet commented 4 years ago

@edc123 Fix landed in 1.0.7