Fix dom_id collisions by using a UUID rather than an autoincremented index#438
Conversation
|
@michaelbaudino This is REALLY OUTSTANDING. We need to update the I'm leaning to agree with you that we'll do 6.0.x for this. However, I'm pretty sure that semantic versioning fanatics would I'm hoping that this only affects tests, and that with a good description in the CHANGELOG.md and this PR as a reference, then the change is minor enough not to cause pain. Also, please squash to one commit. Thanks again!
|
…d index. This ensures that all DOM nodes ids are unique when multiple instances of a component co-exist on the same page. It also allows fragment caching of generated HTML (since the unicity does not depend on other components on the page). :warning: one must use the `id` parameter of `react_component` to force the generated DOM node id in order to have a predictable one (_e.g._ in tests). This commit also includes the CHANGELOG update and the version bump. Solves shakacode#437
866cc44 to
ea3c7b3
Compare
|
OK @justin808 I squashed those commits and added a CHANGELOG update (quite clear, hopefully) and the version bump. Let me know if there's something else to do 👍 |
|
I'm going to merge this. I might change to v7 however because there is work to be done, and I do believe that a 6.0.X change would suggest no chance of breaking tests.
|
This PR ensures that all DOM nodes ids are unique when multiple instances of a component co-exist on the same page.
It also allows fragment caching of generated HTML (since the unicity does not depend on other components on the page).
Solves #437
A few thoughts following discussion in #437 (numbered, to ease followups):
dom_idis not only used in tests, but also to attach React components to DOM nodes (you're probably usingReactDOM.renderor equivalent)/\d+/...idparameter forreact_component(in our case anActiveRecordid, but it could as well be a UUID) and then wraps the call toreact_componentin acache(["v1", I18n.locale, some_dependencies...])blockidforreact_componentcalls that have a display-only purpose?)I hope this helps.
This change is