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

Ignore dangerouslySetInnerHTML during hydration #1595

Merged
merged 3 commits into from
May 16, 2019

Conversation

developit
Copy link
Member

@developit developit commented May 2, 2019

This matches React's behavior.
I can't find the issue right now but I will.

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@coveralls
Copy link

coveralls commented May 2, 2019

Coverage Status

Coverage increased (+0.9%) to 100.0% when pulling ee570d5 on hydrate-dangerouslysetinnerhtml into 98d13cd on master.

@marvinhagemeister
Copy link
Member

Looks like we'd need to remove the should hydrate with dangerouslySetInnerHTML test.

@developit
Copy link
Member Author

Might be worth adding a warning via preact/debug for this too.

@cristianbote
Copy link
Member

Hey @developit, I updated the test that was failing and I was trying to see if I could add the warning part. But no idea how to detect this case, inside debug/. Any pointers?

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet! I think we can merge it without changes to preact/debug. Those are imo low priority and can happen in a separate PR if needed 🎉

@marvinhagemeister marvinhagemeister merged commit f4e7d71 into master May 16, 2019
@marvinhagemeister marvinhagemeister deleted the hydrate-dangerouslysetinnerhtml branch May 16, 2019 22:36
@johannesodland
Copy link

Hey @developit

Is the intention to avoid reaplying innerHTML, or is it to strip away the content of VNodes with dangerouslySetInnerHTML?

When using SSR the current solution strips away all content rendered with dangerouslySetInnerHTML during hydration. This breaks 10.0.0-beta.2 for us.

We use dangerouslySetInnerHTML to render content from our CMS (and have been doing so for years). It renders perfectly on the server, but after 10.0.0-beta.2 the content is stripped away during hydration, essentially leaving the application blank.

@marvinhagemeister
Copy link
Member

@johannesodland Our future approach will to only attach event listeners in hydration and leave the DOM as is. That work is tracked in #1697

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.

None yet

6 participants