hanford / remark-slate

Remark plugin to compile Markdown as a slate 0.50+ compatible object.
155 stars 42 forks source link

List serialization/deserialization issues #28

Closed jthrilly closed 3 years ago

jthrilly commented 3 years ago

Hello! Firstly, thank you for your work on this project.

We noticed two issues with list item behaviour specifically.

The first is that serialized output lacks line breaks between list items (the output looks like -item one-item two-item three\n). Your serialize function applies line breaks to almost all elements, but not list items. We presume this is because the mdast standard (not what Slate uses, but used by remark, and which a lot of plugins seem to have adopted) insists that list items should wrap their text in paragraphs, so your serializer is expecting to apply a line break following a paragraph child node. However Slate currently does not require this structure as of 0.58.0.

Would you accept a PR that adds a line break to the list item if there is no child paragraph? We have tested this in a fork, and it makes the serialization behave as expected.

The second issue is that running remark-parse against markdown generated by your serializer does generate an mdast with paragraphs wrapping text in list items, which the deserializer converts into paragraph nodes in the Slate state format. This in turn causes slate to render list items with paragraphs. This creates a usability issue whereby pressing enter within a list does not add a list item (it creates a line break within a paragraph). The behaviour of remark-parse is "correct" according to the specifications. Their suggestion is that this case should be handled in the parser, which in this case would be the deserialize function of this library. We were able to resolve it by using the mdast-flatten-listitem-paragraphs plugin. It isn't clear if this is an issue that should be resolved here, but it does create the situation where a round trip serialize/deserialize with this package mutates the content in Slate. I am happy to create a separate issue or PR if you would like a fix implementing based on the plugin above.

Sample Slate slate object from 0.59.0: ``` json [ { "type": "paragraph", "children": [ { "text": "Unordered" } ] }, { "type": "ul_list", "children": [ { "type": "list_item", "children": [ { "text": "first" } ] }, { "type": "list_item", "children": [ { "text": "second" } ] }, { "type": "list_item", "children": [ { "text": "third" } ] } ] }, { "type": "paragraph", "children": [ { "text": "Ordered" } ] }, { "type": "ol_list", "children": [ { "type": "list_item", "children": [ { "text": "first" } ] }, { "type": "list_item", "children": [ { "text": "second" } ] }, { "type": "list_item", "children": [ { "text": "third" } ] } ] } ] ```

Screenshot of editor:

Screen Shot 2021-03-24 at 8 15 30 PM

remark-slate serialized output:

Screen Shot 2021-03-25 at 10 33 37 AM

deserialized mdast from remark-parse based on "fixed" markdown

Screen Shot 2021-03-25 at 10 39 50 AM
MDAST output: ``` json [ { "type": "paragraph", "children": [ { "text": "Unordered" } ] }, { "type": "ul_list", "children": [ { "type": "list_item", "children": [ { "type": "paragraph", "children": [ { "text": "first" } ] } ] }, { "type": "list_item", "children": [ { "type": "paragraph", "children": [ { "text": "second" } ] } ] }, { "type": "list_item", "children": [ { "type": "paragraph", "children": [ { "text": "third" } ] } ] } ] }, { "type": "paragraph", "children": [ { "text": "Ordered" } ] }, { "type": "ol_list", "children": [ { "type": "list_item", "children": [ { "type": "paragraph", "children": [ { "text": "first" } ] } ] }, { "type": "list_item", "children": [ { "type": "paragraph", "children": [ { "text": "second" } ] } ] }, { "type": "list_item", "children": [ { "type": "paragraph", "children": [ { "text": "third" } ] } ] } ] } ] ```

Results in broken editor:

Screen Shot 2021-03-25 at 10 40 46 AM
hanford commented 3 years ago

Hey thanks for the detailed issue. I think a second issue is appropriate for the second thing you've brought up.

--

For the first issue, that's really interesting. I'm not opposed to a PR as long as it doesn't dramatically change the input/output of the structures that do have ul > li > p, though I am apprehensive of diverging from the mdast standard.

I use this plugin which enforces the above structure. I'd check it out if you haven't as there are lot of nice functionalities that you get for free.

jthrilly commented 3 years ago

Thanks for the tip @hanford - we actually implemented that plugin in the meantime, and you're right that it works well, and resolves both issues.

Based on this, I propose that a solution to this might be to simply document that the serializer is expecting a Slate state object that is MDAST compliant, meaning paragraphs wrapping list-item text, and that users should consider using the slate-edit-list plugin as one easy way to ensure that. Based on the current readme, a user would expect the more vanilla output from my example to be parsed correctly, since the stated purpose of the utility is to serialize the slate state object as markdown. IMO Slate's state should be normalized to be AST compliant by default, but that's a whole other issue.

Updating this utility to work more broadly might become hard to maintain in the long run, because you would end up replicating the complexity in AST itself, and would be better off reframing this utility as parsing from Slate to a normalized intermediary format, and then to markdown. This seems to have been the approach taken by slate-mdast-serializer (since abandoned), and now taken by remark-slate-transformer, as well as accordproject/markdown-transform/accordproject/markdown-slate. It is also implicitly how your deserializer is working, which is why the round trip mutates the content currently. I think documenting is simpler, and solves the issue from a user-facing perspective..

hanford commented 3 years ago

I added a small blurb about using remark-slate alongside slate-edit-list in the README here, If you'd like to add any copy or change what I wrote I'd happily merge a PR.

Thanks!