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

[WIP] Fix HTML escaping and non-top-level <script> and <style> issues #1086

Merged
merged 7 commits into from
Jan 9, 2018

Conversation

Conduitry
Copy link
Member

Ref #1082.

I'm not thrilled with how some of these changes feel. If we want to support non-top-level <script> and <style> (which don't get special handling from Svelte) we're still going to have to parse them differently, since they're not HTML. Perhaps these should get their own Node subclass? That might not actually make anything tidier.

There's also a possibility that it would be valuable to allow for {{tags}} in these <script> and <style> elements. (In this branch right now their contents are always parsed as a single string until the closing tag.) That would quite possibly be very confusing and have to come with a lot of caveats, especially in DOM mode.

As it stands, the first commit here (in stringified Text nodes, only escape &, <, >) is the only one I feel particularly good about. There aren't even tests yet.

tl;dr I still really don't know what a solution to #1082 would look like.

@codecov-io
Copy link

codecov-io commented Jan 8, 2018

Codecov Report

Merging #1086 into master will decrease coverage by 0.16%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1086      +/-   ##
==========================================
- Coverage   91.69%   91.53%   -0.17%     
==========================================
  Files         123      123              
  Lines        4469     4477       +8     
  Branches     1440     1443       +3     
==========================================
  Hits         4098     4098              
- Misses        157      162       +5     
- Partials      214      217       +3
Impacted Files Coverage Δ
src/utils/stringify.ts 100% <ø> (ø) ⬆️
...nerators/server-side-rendering/visitors/Element.ts 84.84% <0%> (-5.48%) ⬇️
src/generators/nodes/Element.ts 94.42% <0%> (-0.63%) ⬇️
src/parse/state/tag.ts 93.63% <0%> (-2.45%) ⬇️

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 dbe8105...f6e6cb6. Read the comment docs.

@Rich-Harris
Copy link
Member

Good points. I think the important thing is fixing the regression (oops!) and crossing the script/style bridge when we come to it — we're no worse off than we were before. I've added a test for the entities, and tried to add one for the script/style thing but there's some hydration nonsense to sort out first, so I put that in a separate PR (#1087) and we can deal with it some other time.

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