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 non-root <script> hydration #1087

Closed
wants to merge 5 commits into from
Closed

Conversation

Rich-Harris
Copy link
Member

This fails because hydration behaves differently with non-top-level <script> and <style>

@Conduitry
Copy link
Member

Do you have any good ideas about how we want to handle this? As I was poking around at this today, I was gradually realizing that it didn't seem there was a great solution.

@Conduitry
Copy link
Member

Oh, I was confused. I didn't realize you were planning on merging in all of my changes in #1086, I thought you just going to grab the &, <, > escaping one.

@Rich-Harris
Copy link
Member Author

Yeah, I figured it makes more sense than the status quo. Can't quite wrap my brain round what's going on with this test, though I feel like it's something obvious

@Conduitry
Copy link
Member

Took a look at this - I can't tell whether this is something you'd already realized, but it looks to me like the generated code for these non-top-level elements in hydratable components is wrong, not an issue with the test. There's code to attempt to claim the script/style tag, but then nothing to put in its content. I assume we'd want to do this with a claimText?

In my PR #1086, I changed how these elements are parsed so that the script/style contents are stored in the data property right on the tag, and not in a Text child node. I forget why I did that that way. I think because it made the DOM toHTML and the SSR visitor a little simpler? But anyway that means we either need to handle that correctly via another special case in getClaimStatement, or we need to go back to parsing those tags as having a single Text child.

@Conduitry
Copy link
Member

Fixed hydration and got this test to pass! Looks like the failure now is just Travis. I followed a similar approach as in #1162, except that <script> tags will only ever be parsed as one piece of Text, with no template tags. I also merged that branch into this one, because there would have been conflicts and also I doubt the <style> part of this test would have passed without it.

@Conduitry Conduitry changed the title Additional (failing) test for #1082 Fix non-root <script> hydration Feb 8, 2018
@codecov-io
Copy link

Codecov Report

Merging #1087 into master will increase coverage by 0.47%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1087      +/-   ##
==========================================
+ Coverage   91.48%   91.96%   +0.47%     
==========================================
  Files         126      126              
  Lines        4524     4630     +106     
  Branches     1461     1507      +46     
==========================================
+ Hits         4139     4258     +119     
+ Misses        163      160       -3     
+ Partials      222      212      -10
Impacted Files Coverage Δ
src/generators/nodes/Element.ts 95.09% <100%> (+0.65%) ⬆️
src/parse/state/tag.ts 96.36% <100%> (+2.73%) ⬆️
...nerators/server-side-rendering/visitors/Element.ts 91.89% <100%> (+5.4%) ⬆️
src/generators/dom/index.ts 96.04% <0%> (+0.56%) ⬆️
src/css/Stylesheet.ts 94.44% <0%> (+4.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29a1569...61909fb. Read the comment docs.

@Conduitry
Copy link
Member

👍 Everyone's happy now. #1162 should probably be merged before this, or it will have nothing to do.

@Conduitry
Copy link
Member

Still some problems with escaping <, >, & in <style> when we shouldn't. Just going to close this and #1162 and open a new PR to avoid a big git mess.

@Conduitry Conduitry closed this Feb 8, 2018
@Conduitry Conduitry deleted the gh-1082-script-style-test branch February 9, 2018 13:45
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