feat: add `unstable_htmlAsRichText` and `unstable_markdownAsRichText` helpers #342

lihbr · 2024-07-18T18:24:18Z

Implements: RFC: HTML to Rich Text Serializer
Fixes: #331

Description

This pull request implements helpers to serialize HTML and markdown to Prismic rich text field format as described per the related RFC.

These new helpers are introduced to help users migrating to Prismic through the migration API to convert their HTML content to rich text easily.

Checklist

If my changes require tests, I added them.
If my changes affect backward compatibility, it has been discussed.
If my changes require an update to the CONTRIBUTING.md guide, I updated it.
Discuss what we want to expose from where
- Where to export htmlAsRichText, markdownAsRichText, and AsRichTextConfig from
- Whether or not to export filterRichTextField (could be helpful internally)
- Whether or not to export rehypeRichText and hastUtilToRichText

How to QA ¹

Review the test suite introduced by this pull request: https://github.com/prismicio/prismic-client/blob/lh/html-to-richtext-converter/test/richtext/htmlAsRichText.test.ts

Try it yourself creating a playground file on a clone of the repo and running it with sample code

// run with `npx tsx playground.ts`
import { unstable_htmlAsRichText } from "./src";

const result = unstable_htmlAsRichText(/* html */ `<p>hello world</p>`);

console.log(result);

Please use these labels when submitting a review:
❓ #ask: Ask a question.
💡 #idea: Suggest an idea.
⚠️ #issue: Strongly suggest a change.
🎉 #nice: Share a compliment. ↩

- remove `sync` helpers - rename `converter` option to `serializer` - add `direction`, `include` options - remove `silent` option - allow deep selectors in the `serializer` map - refactor helpers return type - refactor `rehypeRichText` plugin - refactor tests

- add, test, and export `filterRichTextField` helper - refactor `htmlAsRichText` and `markdownAsRichText` to be synchronous - support `model` option - support `defaultWrapperNodeType` option - test `markdownAsRichText`

- support function serializers - document all public interfaces - refactor `hastUtiltoRichText` - extract default node serializers to `serializerHelpers` - delete `isNodeType` helpers - finish testing coverage

angeloashmore

Awesome work! It seems to work really well — not an easy feat.

I'm going to mark the PR as "Request changes" since there are a number of comments that require discussion, but that doesn't necessarily mean anything needs to change. I don't have any blocking comments.

❓ General questions

Returning messages as VFileMessage[] ties our API to unified. Although we don't have any immediate plans to transition away from unified, I could see us wanting to explore a version without it for performance and size benefits. What do you think of abstracting VFileMessage[] into a higher-level version with objects that we create?
Should warnings be named messages to match unified's output? I wonder if there would ever be a message that isn't a warning, in which case warnings would not make sense. If the messages are always warnings, warnings is good.
filterRichTextField seems like something that belongs in user code rather than @prismicio/client. It functions as a way to clean up already existing rich text data. It assumes the user has easy access to the model and is working with invalid rich text data for the model. Couldn't the user simply not return invalid rich text nodes in the serializer?
- What happens when rich text data that contains invalid nodes (e.g. a heading1 when Heading 1 is not enabled) is uploaded to Prismic? Does it filter out the invalid nodes automatically? If yes, filterRichTextField may not be necessary for the *AsRichText helper's intended use cases.

src/richtext/types.ts

src/richtext/filterRichTextField.ts

src/richtext/htmlAsRichText.ts

src/richtext/utils/serializerHelpers.ts

angeloashmore · 2024-07-19T01:17:29Z

src/richtext/utils/serializerHelpers.ts

+	let width = (node.properties?.width as number) ?? 0;
+	let height = (node.properties?.height as number) ?? 0;


❓ #question: Do these values mean images will be 0x0 by default? I thought we were leaving the values off if we couldn't infer it.

I updated them to undefined but I had to cast them as number down the road because the API types are always filled. Does that sound OK to you? Should we update the API types instead to allow undefined?

Hm… the public API always returns dimension values so we should keep them as always defined.

How do we know the Migration API doesn't require them? If the API really doesn't require them, then casting is good with me. When we start working on the Migration API client, we may want to look into a custom Migration API-specific ImageField type.

Updated to cast! That's what @Heyrows shared with me for reference: https://github.com/prismicio/migration-api/blob/5a8455118c5fcbcb60225a94919a97cc7646fd73/src/models/import/validators/fields/nestable/Image/merge.ts#L35-L38, they leave them empty and compute them later on.

I don't have an issue with having a separate ImageField type, but then that means we also need to make the rich text types and nodes more flexible to accommodate both or duplicate them, which I feel meh about 🤔

Added a small comment about it linking to this thread 👌

src/richtext/utils/serializerHelpers.ts

test/__testutils__/testAsRichTextHelper.ts

lihbr · 2024-07-19T15:15:30Z

Thank you so much for the prompt review! I updated everything as per it and answered discussions ☺️

Returning messages as VFileMessage[] ties our API to unified. Although we don't have any immediate plans to transition away from unified, I could see us wanting to explore a version without it for performance and size benefits. What do you think of abstracting VFileMessage[] into a higher-level version with objects that we create?

I updated the signature to be an array of string :)

Should warnings be named messages to match unified's output? I wonder if there would ever be a message that isn't a warning, in which case warnings would not make sense. If the messages are always warnings, warnings is good.

VFile has 3 levels of log: info (regular log), messages (warnings), and fatal (error). I think warnings makes it clearer in our case.

filterRichTextField seems like something that belongs in user code rather than @prismicio/client. It functions as a way to clean up already existing rich text data. It assumes the user has easy access to the model and is working with invalid rich text data for the model. Couldn't the user simply not return invalid rich text nodes in the serializer?

Discussed here: #342 (comment), this is a bit trickier than not providing a serializer for the given type of node I'm afraid.

What happens when rich text data that contains invalid nodes (e.g. a heading1 when Heading 1 is not enabled) is uploaded to Prismic? Does it filter out the invalid nodes automatically? If yes, filterRichTextField may not be necessary for the *AsRichText helper's intended use cases.

We'll discover that pretty soon as I start to work with the migration API I assume 🤔

lihbr · 2024-07-22T19:10:28Z

We agreed to export everything from @prismicio/client directly.

richtext/utils will be moved to lib
filterRichTextField.ts will be moved to lib

src/richtext/utils/RichTextFieldBuilder.ts

angeloashmore · 2024-07-22T20:16:36Z

src/richtext/utils/hastToRichText.ts

 */
-export type HastUtilToRichTextConfig = {
+export type HASTToRichTextConfig = {


💡 #idea: I think this type should be written as HastToRichTextConfig since hast is not an acronym like HTML.

From GitHub, it's the acronym for Hypertext Abstract Syntax Tree, so I'll leave it as-is, but we can rename it later.

See how letters are bolded at the very start of the README: https://github.com/syntax-tree/hast

angeloashmore

This looks great! ❤️

As we discussed, we just need to export the helpers from the right entries and reorganize some of the lib functions.

Tree-shaking works as we want: there are no traces of the unified ecosystem if you don't use the *AsRichText helpers. ~48 KB was added when using the markdownAsRichText helper. As a general rule, we'll recommend against using the helper on the client.

Co-authored-by: Angelo Ashmore <[email protected]>

lihbr · 2024-07-23T12:46:37Z

src/richtext/index.ts

@@ -4,6 +4,7 @@ export { asText } from "./asText";
 export { serialize } from "./serialize";
 export { wrapMapSerializer } from "./wrapMapSerializer";
 export { composeSerializers } from "./composeSerializers";
+export { filterRichTextField } from "../lib/filterRichTextField";


I removed that on master 🙈

lihbr added 8 commits July 11, 2024 21:11

feat: add htmlAsRichText, markdownAsRichText helpers

8aa591b

chore: remove playground

b9676c1

refactor: don't use findLastIndex()

aa0180d

feat: update htmlAsRichText API

8fe0a49

- add, test, and export `filterRichTextField` helper - refactor `htmlAsRichText` and `markdownAsRichText` to be synchronous - support `model` option - support `defaultWrapperNodeType` option - test `markdownAsRichText`

feat: update htmlAsRichText API

f8ad9fe

- support function serializers - document all public interfaces - refactor `hastUtiltoRichText` - extract default node serializers to `serializerHelpers` - delete `isNodeType` helpers - finish testing coverage

test: test warnings on all cases

5cff6b3

feat: support nullish values for html rich text serializer functions

eff843a

lihbr requested a review from angeloashmore July 18, 2024 19:01

angeloashmore requested changes Jul 19, 2024

View reviewed changes

refactor: per review

09fcff8

lihbr mentioned this pull request Jul 22, 2024

Type for TitleField missing direction field (mismatch in typings versus data from your endpoint) #331

Closed

lihbr added 7 commits July 22, 2024 16:45

refactor: mark new helpers as unstable_

154b910

refactor: don't accept null in rich text html serializer

81eb7b7

refactor: renamed hastUtilToRichText to hastToRichText

181d854

feat: remove defaultWrapperNodeType option

7d872b9

refactor: simplify DistributedOmit and DistributedPick

ade5cc4

refactor: throw custom error classes for clarity

c73878a

docs: comment image dimensions casting

0184de9

angeloashmore reviewed Jul 22, 2024

View reviewed changes

src/richtext/utils/RichTextFieldBuilder.ts Outdated Show resolved Hide resolved

angeloashmore reviewed Jul 22, 2024

View reviewed changes

angeloashmore approved these changes Jul 22, 2024

View reviewed changes

lihbr and others added 2 commits July 23, 2024 13:35

docs: tsdocs

7ab5a9c

Co-authored-by: Angelo Ashmore <[email protected]>

refactor: finalize exports

668fefd

lihbr changed the title ~~feat: add htmlAsRichText, markdownAsRichText helpers~~ feat: add unstable_htmlAsRichText and unstable_markdownAsRichText helpers Jul 23, 2024

lihbr merged commit 8c5a82b into master Jul 23, 2024
12 checks passed

lihbr deleted the lh/html-to-richtext-converter branch July 23, 2024 12:45

lihbr commented Jul 23, 2024

View reviewed changes

lihbr mentioned this pull request Jul 25, 2024

style: update eslint config #343

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `unstable_htmlAsRichText` and `unstable_markdownAsRichText` helpers #342

feat: add `unstable_htmlAsRichText` and `unstable_markdownAsRichText` helpers #342

lihbr commented Jul 18, 2024 •

edited

Loading

angeloashmore left a comment

angeloashmore Jul 19, 2024

lihbr Jul 19, 2024

angeloashmore Jul 19, 2024

lihbr Jul 22, 2024

lihbr Jul 22, 2024

lihbr commented Jul 19, 2024

lihbr commented Jul 22, 2024

angeloashmore Jul 22, 2024 •

edited

Loading

lihbr Jul 23, 2024 •

edited

Loading

angeloashmore left a comment

lihbr Jul 23, 2024

		let width = (node.properties?.width as number) ?? 0;
		let height = (node.properties?.height as number) ?? 0;

feat: add unstable_htmlAsRichText and unstable_markdownAsRichText helpers #342

feat: add unstable_htmlAsRichText and unstable_markdownAsRichText helpers #342

Conversation

lihbr commented Jul 18, 2024 • edited Loading

Description

Checklist

How to QA 1

Footnotes

angeloashmore left a comment

Choose a reason for hiding this comment

❓ General questions

angeloashmore Jul 19, 2024

Choose a reason for hiding this comment

lihbr Jul 19, 2024

Choose a reason for hiding this comment

angeloashmore Jul 19, 2024

Choose a reason for hiding this comment

lihbr Jul 22, 2024

Choose a reason for hiding this comment

lihbr Jul 22, 2024

Choose a reason for hiding this comment

lihbr commented Jul 19, 2024

lihbr commented Jul 22, 2024

angeloashmore Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

lihbr Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

angeloashmore left a comment

Choose a reason for hiding this comment

lihbr Jul 23, 2024

Choose a reason for hiding this comment

feat: add `unstable_htmlAsRichText` and `unstable_markdownAsRichText` helpers #342

feat: add `unstable_htmlAsRichText` and `unstable_markdownAsRichText` helpers #342

lihbr commented Jul 18, 2024 •

edited

Loading

How to QA ¹

angeloashmore Jul 22, 2024 •

edited

Loading

lihbr Jul 23, 2024 •

edited

Loading