fix external type definitions of renderToString

[Geo25rey]

Ran into type errors when running the following:

renderToString(h(RootComponent, null));

This PR should fix it.

After checking if this change has any runtime implications, it seems that only Fragments are affected. So, I've added a fix for that as well. Not sure why anyone would want a Fragment with null props though.

[changeset-bot]

🦋 Changeset detected

Latest commit: 87d8c21

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
preact-render-to-string	Patch

Not sure what this means? Click here to learn what changesets are.

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

[Geo25rey]

Sorry about the messy changeset commits. This is only the second time I've used a changeset.

[rschristian]

After checking if this change has any runtime implications, it seems that only Fragments are affected. So, I've added a fix for that as well. Not sure why anyone would want a Fragment with null props though.

What exactly is the problem here? Should work as-is, I believe?

Sorry about the messy changeset commits. This is only the second time I've used a changeset.

All good, no worries! I think the types change can land as a patch? Or is there some usage that might break?

[Geo25rey]

What exactly is the problem here? Should work as-is, I believe?

This code would pass type checking but would lead to a runtime TypeError: Cannot read property tpl of null:

renderToString(h(Fragment, null));

Yes, this is useless, but it passes type checking and it would be a super weird bug to track down later if there is a use case for this that I am unaware of.

All good, no worries! I think the types change can land as a patch?

According to https://semver.org,

MINOR version when you add functionality in a backward compatible manner

I figure this applies here. Unless, there is a different definition that I am not aware of.

Or is there some usage that might break?

No, this should just add functionality.

[rschristian]

What exactly is the problem here? Should work as-is, I believe?

This code would pass type checking but would lead to a runtime TypeError: Cannot read property tpl of null:

renderToString(h(Fragment, null));

Looks fine to me? REPL Demo

All good, no worries! I think the types change can land as a patch?

According to https://semver.org,

MINOR version when you add functionality in a backward compatible manner

I figure this applies here. Unless, there is a different definition that I am not aware of.

There's no functionality being added in correcting types. Adding functionality would be adding a new method or something.

As types aren't really part of the public API, breaking types changes can land as a minor in most libs which is why I asked if there was some breaking scenario. But sounds like we're all good there.

[Geo25rey]

What exactly is the problem here? Should work as-is, I believe?

This code would pass type checking but would lead to a runtime TypeError: Cannot read property tpl of null:
renderToString(h(Fragment, null));

Looks fine to me? REPL Demo

Looking at the Preact VNode type, it seems that props is guaranteed to be an object with some children. This makes the Fragment.props check redundant. I'll remove it.

export interface VNode<P = {}> {
  type: ComponentType<P> | string;
  props: P & { children: ComponentChildren };
  ...
}

All good, no worries! I think the types change can land as a patch?

According to https://semver.org,

MINOR version when you add functionality in a backward compatible manner

I figure this applies here. Unless, there is a different definition that I am not aware of.

There's no functionality being added in correcting types. Adding functionality would be adding a new method or something.

As types aren't really part of the public API, breaking types changes can land as a minor in most libs which is why I asked if there was some breaking scenario. But sounds like we're all good there.

Types are part of the public API (application programming interface). Interface means not only how you use it but also the inline type documentation on how to use it.

For example, if, instead of the <P = {}>, <P extends string> was the type parameter, many renderToString users' code would break since it would have a compile error. This would be indicative of a major version bump even if the JavaScript runtime behavior hasn't changed.

I think this PR is a step down from that since it has no breaking behavior but adds the ability for props to be null or never.

Consider the RootComponent:

function RootComponent() {
  return <div></div>;
}

The type of props for this should be never. See playground example.

[rschristian]

Looking at the Preact VNode type, it seems that props is guaranteed to be an object with some children

I wouldn't necessarily put too much weight/trust into those types, they'll generally work fine but there are some rough edges 😅

Types are part of the public API (application programming interface). Interface means not only how you use it but also the inline type documentation on how to use it.

For better or for worse, that isn't really the case. Any change to the types would inherently be a major -- there is no fully backwards compatible type change. Including it as part of the semver process is therefore something no library truly does.

Should be fine as a patch in this case IMO.

[Geo25rey]

Looking at the Preact VNode type, it seems that props is guaranteed to be an object with some children

I wouldn't necessarily put too much weight/trust into those types, they'll generally work fine but there are some rough edges 😅

Yeah, I saw when I looked at the implementation. There are some pros of the relaxed types like human readability.

For better or for worse, that isn't really the case. Any change to the types would inherently be a major -- there is no fully backwards compatible type change. Including it as part of the semver process is therefore something no library truly does.

Should be fine as a patch in this case IMO.

So would a breaking type change be a minor change for just preact-render-to-string or all preact packages in general?

[rschristian]

Thanks!

[rschristian]

So would a breaking type change be a minor change for just preact-render-to-string or all preact packages in general?

I'd call that a minor in any package, not just Preact, but yeah, we generally follow that.

Types (especially in JS-first libs) are a best-effort to reflect the runtime constraints. If they're incorrect, that's a bug, and semver is a-okay with fixing bugs (even those a user might rely upon) in a patch (but I'd argue for a minor if it could be reasonably relied upon). Obviously we don't want to break anyone's setup, but pushing out majors to fix them being out of sync would be a bit much.