Attribute hook render option

[gpoitch]

Adds an attribute hook rendering option. This allows the user to customize how attribute name casing is rendered.
There have been several issues and attempts over years to implement this. Namely: #199

Seems the consensus is to do nothing because other frameworks don't and there isn't an agreeable set of regexes to handle everything, so instead let the user do it.

From my experience, the proper casing on some of these is absolutely critical. We've been maintaining a fork for years, and couldn't keep up syncing upstream improvements after v6 so this is a more non-invasive approach.

[changeset-bot]

⚠️ 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.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

[JoviDeCroock]

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

import { options } from 'preact';

const oldVnodeHook = options.vnode
options.vnode = (vnode) => {
  // is it a dom node?
  if (vnode.type === 'string') {
    const newProps = {};
    // iterate over props and do stuff
    vnode.props = newProps
  }
  if (oldVnodeHook) oldVnodeHook(vnode);
}

[marvinhagemeister]

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

[gpoitch]

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

[marvinhagemeister]

@gpoitch Your codesandbox has a couple of issues and unnecessary cloning. The options object is a global and can be thought of as global event listeners. By attaching a new options.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 same props object over and over again. Doing it once is enough.

I'm not sure why the options object is cloned to defaultOptions. Maybe that's a leftover from an earlier exploration? The .reduce() call can be simplified by using a for-in loop. The wrapper around renderToString 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?

[gpoitch]

@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

[marvinhagemeister]

• 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.

[gpoitch]

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.

[marvinhagemeister]

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 in preact-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?

[gpoitch]

Nothing uses uppercase. Again, that was for a simple example's sake. All of the real transformations I've encountered are 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 make this the default. If you have another direction in mind, I will gladly implement it.

[marvinhagemeister]

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?

[gpoitch]

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