Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 2, 2016

Conversation

michaelbaudino
Copy link
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

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

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