⚠️ No Changeset found

Latest commit:ed1913e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go.If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Hey, isn't this possible with usingoptions.vnode? In your server file you would write something like the following

import{options}from'preact';constoldVnodeHook=options.vnodeoptions.vnode=(vnode)=>{// is it a dom node?if(vnode.type==='string'){constnewProps={};// iterate over props and do stuffvnode.props=newProps}if(oldVnodeHook)oldVnodeHook(vnode);}

Agree, to me it feels like that use case is already covered byoptions.vnode. Benefit of using that is that it also works in the browser if it's included in a script tag somewhere.

It doesn't really work well.

• You have to reimplement everything here:https://github.com/preactjs/preact-render-to-string/blob/main/src/index.js#L323-L399
• Modifying those options has a weird non-idempotent state between renders.https://codesandbox.io/s/nervous-tdd-vcyn7m

@gpoitch Your codesandbox has a couple of issues and unnecessary cloning. Theoptions object is a global and can be thought of as global event listeners. By attaching a newoptions.vnode hook on every render you're growing the chain of hooks by one with each render. There is no need to try to uppercase the sameprops object over and over again. Doing it once is enough.

I'm not sure why theoptions object is cloned todefaultOptions. Maybe that's a leftover from an earlier exploration? The.reduce() call can be simplified by using afor-in loop. The wrapper aroundrenderToString doesn't do anything either.

Here is a cleaned up version of the codesandbox:https://codesandbox.io/s/intelligent-heisenberg-7h6ysl?file=/src/index.js

You have to reimplement everything here:main/src/index.js#L323-L399

I guess the more interesting question we should ask here is: Why do you want to uppercase the attribute names in the first place? I think if you can present a case where this is needed we're more than happy to add it. Can you share more about that?

@marvinhagemeister the uppercasing was just to keep the example simple.
The wrapper was hopefully for installing different hooks when rendering different content, which doesn't appear possible with the options modifications. That's why I was messing with cloning it - I wasn't able to re-set it without a full server/sandbox restart.

Real use-cases for this off the top of my head:

• RSS. Doesn't validate/work in some readers without proper casing
• SVGs not rendering correctly
• AMP validation fails
• srcSet issues in certain browsers downloading wrong/excessive images

• RSS. Doesn't validate/work in some readers without proper casing

That's a fair point. Are those readers common? And is the spec case sensitive too or is it more a reader not following the spec?

• SVGs not rendering correctly

Do you have an example of an SVG that renders incorrectly?

• AMP validation fails

Do you have an example where AMP validation fails?

• srcSet issues in certain browsers downloading wrong/excessive images

To me that sounds like a bug we should fix.

Do you have an example

Yes a media company that runs on this with 10s of millions of daily views 😄. I can put together examples if you really want but there is an entire class of content rendering that may not have preact/the browser "correcting" it.

I'd happily use the vnode option if it's viable but the already handled special cases and option re-modification doesn't look promising. We will continue to run the fork, just thought I'd share the idea since there are several issues about this always popping up (#26,#275,#18,#55). Thanks for taking the time to look at this.

Do you have an example

Yes a media company that runs on this with 10s of millions of daily views 😄. I can put together examples if you really want > but there is an entire class of content rendering that may not have preact/the browser "correcting" it.

It's great to hear that you have successful clients. I was asking for a technical example. I don't work frequently with RSS readers so my question was more about if that's spec thing in that it only allows uppercase attribute names or if the spec is case insensitive and the readers implemented it incorrectly.

Regarding#26,#275,#18,#55: I'd rather fix those issues inpreact-render-to-string itself rather than giving users a hook to work around these. That way everyone can profit from the same fixes. What do you think?

Nothing uses uppercase. Again, that was for a simple example's sake. All of the real transformations I've encounteredare in the tests and#199

The technical examples are literally every single RSS, SVG, and AMP page you write in JSX and render with this library. If the RSS doesn't fully validate, clients/apps won't accept your content. If the AMP doesn't validate, google won't put you in search. If the SVG doesn't have proper attributes it won't render correctly.

Regarding the other issues and correcting the attributes directly, I already provide that in#199 and got no traction. We could do a combination of the 2 - provide this hook and either export or makethis the default. If you have another direction in mind, I will gladly implement it.

Just merged#308 which addresses the known HTML + SVG scenarios. I'm not too familiar with AMP or RSS. Do they have additional cases to be aware of?

For RSS:xmlnsFoo ->xmlns:foo
/^(xlink|xml|xmlns)([A-Z])/ is the regex I was using to colon-ize
(real attrs: xmlns:dc, xmlns:content, xmlns:atom, xmlns:media)

AMP should be handled with the HTML scenarios