feat: pass HTML serializer children as string

[angeloashmore]

Types of changes

• Chore (a non-breaking change which is related to package maintenance)
• Bug fix (a non-breaking change which fixes an issue)
• New feature (a non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Description

This PR changes behavior with custom Rich Text HTML serializers. Rather than pass children as an array of strings, we can pass children as a concatenated string. This is done under the assumption that since we are returning HTML, a single string is always desired.

This saves the user the step of calling String.prototype.join themselves.

The following HTML serializers are now valid:

export const htmlMapSerializer = {
	heading1: ({ children }) => `<h2>${children}</h2>`,
};

export const htmlFunctionSerializer: HTMLFunctionSerializer = (
	_type,
	node,
	_text,
	children,
) => {
	switch (node.type) {
		case Element.heading1: {
			return `<h2>${children}</h2>`;
		}
	}

	return null;
};

Without this PR, children would need to be converted to a string via children.join('').

Checklist:

• My change requires an update to the official documentation.
• All TSDoc comments are up-to-date and new ones have been added where necessary.
• All new and existing tests are passing.

[lihbr]

OK, I'm confused. Don't you want to just update @prismicio/richtext to just pass children as a string there? https://github.com/prismicio/prismic-richtext/blob/v2/src/serialize.ts#L40

The excessive wrapping here looks weird to me 🤔

[angeloashmore]

It depends where we want to do the string concatenation. We can't just call children.join in @prismicio/richtext since not every type should be joined. React components, for example, need to remain as arrays.

We could conditionally check within @prismicio/richtext for a string children type. If we see it is a string, we join it.

I think from a "purity" sense, however, we should keep custom behavior outside the low-level library.

[lihbr]

OK got it! Totally missed that children can be an array of anything. Therefore indeed @prismicio/richtext should not start to handle those cases!

[lihbr]

LGTM, just a quick refactor maybe, you might already have ruled it out for some reasons I missed though 🤔

[lihbr]

What do you think about just updating the existing keys of the provided map serializer here? Something along the following:

const prepareHTMLMapSerializer = (serializer: HTMLMapSerializer): RichTextFunctionSerializer<string> => {
	return wrapMapSerializer(Object.entries(serializer).reduce((acc, [key, value]) => {
		if (value) {
			acc[key] = (payload) => value({ ...payload, children: payload.children.join("") });
		}

		return acc;
	}, {}));
}

Could help with bundle size without impacting too much performances since the map is not meant to grow over the available tags?

[angeloashmore]

The only reason I went with the big object is to resolve type errors. I just tried again with the shorter reducer/for...in method and still couldn't find a way to get it to work without @ts-expect-error.

If you have any ideas on how to do it with working types, I'd love to use that version instead!

[lihbr]

I'll give it a try ☺️ TS is quite messed up with lambdas :(

[lihbr]

OK, this is painful indeed, cannot get beyond that because it tries to merge every parameters possible:

const prepareHTMLMapSerializer = (
	serializer: HTMLMapSerializer
): RichTextFunctionSerializer<string> => {
	return wrapMapSerializer(
		(Object.entries(serializer) as [keyof HTMLMapSerializer, HTMLMapSerializer[keyof HTMLMapSerializer]][])
			.reduce<RichTextMapSerializer<string>>(
				(acc, [key, value]) => {
					if (value) {
						acc[key] = (payload) => value({ ...payload, children: payload.children.join("") });
					}

					return acc;
				},
				{}
			)
	);
};

[angeloashmore]

Thanks for reviewing! I'll leave this as a draft for now. I'm not totally happy with the excessive prepareHTMLMapSerializer and withStringifiedChildren functions. I feel like there's a better way we could do it, I'm just not sure how yet.

[lihbr]

Yup, kinda share the same feeling too~

I think we experienced the same issue already with wrapMapSerializer and ended up using a bunch of @ts-expect-error there to get away from the never intersections 🤔

https://github.com/prismicio/prismic-richtext/blob/v2/src/wrapMapSerializer.ts#L29-L42

(we could have done a huge switch statement but well 🤷 bundle size)

[angeloashmore]

Ah yes, that looks like the same issue. Although it isn't ideal, I prefer keeping the bundle size down by using reduce or for...in with @ts-expect-error. Our tests should make sure all serializer block types work with string children.

Thanks, I'll make these changes today and leave it open for review.

[angeloashmore]

@lihbr Ready for review!