Skip to content

Fix dom_id collisions by using a UUID rather than an autoincremented index#438

Merged
justin808 merged 1 commit intoshakacode:masterfrom
michaelbaudino:fix-dom_id-collisions
Jun 2, 2016
Merged

Fix dom_id collisions by using a UUID rather than an autoincremented index#438
justin808 merged 1 commit intoshakacode:masterfrom
michaelbaudino:fix-dom_id-collisions

Conversation

@michaelbaudino
Copy link
Copy Markdown
Contributor

@michaelbaudino michaelbaudino commented May 31, 2016

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):

  1. If I understand correctly, dom_id is not only used in tests, but also to attach React components to DOM nodes (you're probably using ReactDOM.render or equivalent)
  2. I did not implement the proc version, sorry 😞 (but I'm not sure it would really be useful, tbh)
  3. I'd consider my implementation backward-compatible (since it only changes internal behaviour: I consider auto-generated ids to be internal) so I don't think a major bump is needed (at least, if you're using semantic versioning)... OK, actually, I see 1 way of considering generated HTML to be broken: if some users are looking for ids using an hard-coded value (like all the integration tests 😢) or a number-only regexp like /\d+/...
  4. Regarding fragment caching, we simply use a unique identifier as id parameter for react_component (in our case an ActiveRecord id, but it could as well be a UUID) and then wraps the call to react_component in a cache(["v1", I18n.locale, some_dependencies...]) block
  5. I'm not sure this is how you expected me to make the integration tests pass, let me know if it needs to be changed (maybe we should not explicitly define id for react_component calls that have a display-only purpose?)

I hope this helps.


This change is Reviewable

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.1%) to 82.305% when pulling 866cc44 on michaelbaudino:fix-dom_id-collisions into c7b9209 on shakacode:master.

@justin808
Copy link
Copy Markdown
Member

@michaelbaudino This is REALLY OUTSTANDING.

We need to update the /CHANGELOG.md per prior examples AND explain what you might need to do if you did something like wrote tests that depended on the auto-generated component id.

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
suggest that we bump to 7.0.0 for this.

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!

Previously, coveralls wrote…

Coverage Status

Coverage decreased (-0.1%) to 82.305% when pulling 866cc44 on michaelbaudino:fix-dom_id-collisions into c7b9209 on shakacode:master.


Reviewed 21 of 21 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

…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
@michaelbaudino michaelbaudino force-pushed the fix-dom_id-collisions branch from 866cc44 to ea3c7b3 Compare June 1, 2016 05:58
@michaelbaudino
Copy link
Copy Markdown
Contributor Author

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 👍

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.1%) to 82.305% when pulling ea3c7b3 on michaelbaudino:fix-dom_id-collisions into c7b9209 on shakacode:master.

@justin808
Copy link
Copy Markdown
Member

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.

Previously, coveralls wrote…

Coverage Status

Coverage decreased (-0.1%) to 82.305% when pulling ea3c7b3 on michaelbaudino:fix-dom_id-collisions into c7b9209 on shakacode:master.


Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@justin808 justin808 merged commit 7adc866 into shakacode:master Jun 2, 2016
@michaelbaudino michaelbaudino deleted the fix-dom_id-collisions branch June 3, 2016 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants