ryansolid / dom-expressions

A Fine-Grained Runtime for Performant DOM Rendering
MIT License
850 stars 122 forks source link

Correctly escape style attribute in SSR output #300

Closed danieltroger closed 6 months ago

danieltroger commented 6 months ago

Fixes the behavior that you can observe here: https://stackblitz.com/edit/solid-ssr-vite-juconn?file=src%2Fpages%2FHome.tsx where if you put a string that contains a double quote into the style attribute of an element, the server side rendered version isn't correctly escaped. Example: <main data-hk="0.0.0.2.13.0.0.6.0.0.0.0.0.1"><h2 style="--my-var: url("./test.svg");">Home!</h2></main>

Edit: fix wasn't as easy as I thought, gotta improve this a lil

danieltroger commented 6 months ago

@ryansolid I just noticed this happens with every attribute set in the style of

attribute={`something "with double quotes"`}

Is this enough of a bug report for you? Would appreciate if you could take over this since the issue seems so widespread and I'm unsure how to solve it without further investigation since it seems like it's actually a babel transform issue?

Screenshot 2023-12-26 at 13 53 30
lxsmnsyc commented 6 months ago

The dots on the hydration key indicates an outdated version.

But I guess it's true that quotes doesn't seem to get escaped.

danieltroger commented 6 months ago

Yeah I had the issues in solid 1.8.7, the reproduction was just something I pulled out of google to not have to set up an env

lxsmnsyc commented 6 months ago

Fix should be somewhere here: https://github.com/ryansolid/dom-expressions/blob/b1c5b9d7d9c406070c30d0772eaf9b0e8266f033/packages/babel-plugin-jsx-dom-expressions/src/ssr/element.js#L128

escapeHTML could possibly replaced with escapeExpression however I'm not sure yet how this stuff goes

Never mind I just realized it's the same function lol

lxsmnsyc commented 6 months ago

I'm fine merging this one, and then we could just check on the remaining issue with attributes

danieltroger commented 6 months ago

Sure, sounds good. I'm running this change with yarn patch rn and it fixes the issue with the style attribute for me

But please make sure it gets fixed for the other attributes still :)

danieltroger commented 6 months ago

Just remembered this - did you make sure to fix the escaping of the attributes for other attributes too or should I open an issue to make sure you don't forget it?