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

data-dom-id collision with (fragment) caching #437

Closed
michaelbaudino opened this issue May 30, 2016 · 6 comments
Closed

data-dom-id collision with (fragment) caching #437

michaelbaudino opened this issue May 30, 2016 · 6 comments

Comments

@michaelbaudino
Copy link
Contributor

We encountered a problem where data-dom-id of components on the same page are not unique.

This is particularly obvious when components are stored in cache fragments and can be ordered differently depending on parameters. For exemple, think about search results where each result is a cache fragment containing a component rendered using react_component (the fragment will contain the data-dom-id auto-incremented based on the rank this result had in the page when rendered the first time).

For the record, if some people have the same problem, the workaround we found is to explicitely define data-dom-id using the id parameter of the react_component method as documented here.

The point of this PR is that the default data-dom-id should actually not be auto-incremented based on the rank of the component in the page, but rather by an UUID or (even better, but not sure if possible) by a MD5/SHA hash of its content.

PS: I saw a few existing issues related to fragment caching, but it was more about cache invalidation, not that kind of collision.

@justin808
Copy link
Member

@michaelbaudino Thanks for reporting this. We should get a fix out ASAP. If you have any other feedback, please let me know. A PR would definitely be appreciated.

@justin808
Copy link
Member

@michaelbaudino We probably don't actually need the auto-generated DOM ID for anything other than integration tests (what do we need this for?):

Let's have a config setting:

dom_id_generator: Values:

  • false: (default) which means none
  • UUID (don't really know about MD5)
  • Allow the value to be a proc, so you can have a custom function, and we provide an example of this that gives the old behavior, which could be used just for tests, so that there's a super simple upgrade path for those apps depending on this.

@jbhatab @thewoolleyman @alexfedoseev @robwise Any comments?

Feedback is welcome.

We'll need a major version bump for this.

In the very short term, we need a doc PR to address this. We should have a doc file for Fragment Caching tips. (any volunteers?: call it /docs/additional-reading/fragment-caching.md)

@michaelbaudino Please share your tips on how you set up your cache key with react_on_rails. I'd like to consider eventually making a helper for this.

michaelbaudino added a commit to michaelbaudino/react_on_rails that referenced this issue May 31, 2016
…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).

Solves shakacode#437
@michaelbaudino
Copy link
Contributor Author

michaelbaudino commented May 31, 2016

@justin808 You mean, kinda like #438 ? 😄

@justin808
Copy link
Member

Yes, for anybody reading this. See #438. Big thanks to @michaelbaudino for fixing this!

michaelbaudino added a commit to michaelbaudino/react_on_rails that referenced this issue Jun 1, 2016
…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
@justin808
Copy link
Member

I want to get this one merged. The only thing holding me back is getting consensus on doing a major version bump. I think it's prudent to do a major version bump so somebody doing updates will look at the CHANGELOG.md.

@michaelbaudino Agree?

@michaelbaudino
Copy link
Contributor Author

Sure, great ! Thanks a lot ✨

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

No branches or pull requests

2 participants