Empty List Items throw errors in Slate fromMarkdown

[jolanglinais]

Describe the bug
Error occurs when translating markdown to Slate

Additional context
This is happening when taking markdown from a database, which includes an unordered list (ulist) with a final bullet point which is empty.

The error

Uncaught Error: Node Item doesn't have any children!
    at ToSlateVisitor.processChildNodes (VM84894 ToSlateVisitor.js:1809)
    at ToSlateVisitor.visit (VM84894 ToSlateVisitor.js:2091)
    at ValidatedConcept.accept (VM84924 typed.js:57)
    at eval (VM84894 ToSlateVisitor.js:1820)
    at Array.forEach (<anonymous>)
    at ToSlateVisitor.processChildNodes (VM84894 ToSlateVisitor.js:1815)
    at ToSlateVisitor.visit (VM84894 ToSlateVisitor.js:2079)
    at ValidatedConcept.accept (VM84924 typed.js:57)
    at eval (VM84894 ToSlateVisitor.js:1820)
    at Array.forEach (<anonymous>)

From this source code

processChildNodes(thing) {
    cov_2olvdi9vnx.f[4]++;
    const result = (cov_2olvdi9vnx.s[23]++, []);
    cov_2olvdi9vnx.s[24]++;

    if (!thing.nodes) {
      cov_2olvdi9vnx.b[7][0]++;
      cov_2olvdi9vnx.s[25]++;
      throw new Error("Node ".concat(thing.getType(), " doesn't have any children!"));
    } else {
      cov_2olvdi9vnx.b[7][1]++;
    }

When using this markdown

This is the contract editor. You can edit this text!

This is a heading!
====

This is [text](http://clause.io "").

Fragile Goods
----

`\`\`\ <clause src="ap://fragile-goods@0.12.1#..." clauseid="49c95009-..."/>
Liquidated Damages for Delayed Delivery.

In the event the EXW delivery date of the Equipment is delayed beyond the delivery schedule as indicated below, solely through the fault of <variable id="seller" value="%22Dan%22"/> (the Seller), and unless the parties mutually agreed to an extension thereto, <variable id="buyer" value="%22Steve%22"/> (the Buyer) is entitled to claim liquidated damages in an amount equivalent to <variable id="lateDeliveryPenalty" value="300%20USD"/>.
Prior to implementing the provisions of Article 16.4 pursuant to this section, Buyer agrees that it shall discuss with Seller alternate remedies in good faith.. . . .

The Equipment to be shipped to the Buyer shall be packed and shipped in accordance with the Specifications and if not specified therein....
Additionally the Equipment should have proper devices on it to record any shock during transportation as any instance of acceleration outside the bounds of <variable id="accelerationMin" value="-0.5"/>g and <variable id="accelerationMax" value="0.5"/>g.
Each shock shall reduce the Contract Price by <variable id="accelerationBreachPenalty" value="5%20USD"/>. Packing containing fragile materials should be so marked in bold stout letters. . . . .

Equipment Description, Contract Price and Delivery Schedule

Contract Price is <variable id="deliveryPrice" value="1000%20USD"/> per unit of Equipment.
Delivery Schedule: no later than <variable id="deliveryLimitDuration" value="10%20seconds"/> after initiation.
`\`\`\



- one
- two
- thre
- 

Acceptance of Delivery
----

`\`\`\ <clause src="ap://acceptance-of-delivery@0.12.1#..." clauseid="eb77e820-..."/>
Acceptance of Delivery. <variable id="shipper" value="%22Dan%20Selman%22"/> will be deemed to have completed its delivery obligations if in <variable id="receiver" value="%22Adrian%20Fletcher%22"/>'s opinion, the <variable id="deliverable" value="%22Widgets%22"/> satisfies the Acceptance Criteria, and <variable id="receiver" value="%22Adrian%20Fletcher%22"/> notifies <variable id="shipper" value="%22Dan%20Selman%22"/> in writing that it is accepting the <variable id="deliverable" value="%22Widgets%22"/>.

Inspection and Notice. <variable id="receiver" value="%22Adrian%20Fletcher%22"/> will have <variable id="businessDays" value="10"/> Business Days' to inspect and evaluate the <variable id="deliverable" value="%22Widgets%22"/> on the delivery date before notifying <variable id="shipper" value="%22Dan%20Selman%22"/> that it is either accepting or rejecting the <variable id="deliverable" value="%22Widgets%22"/>.

Acceptance Criteria. The "Acceptance Criteria" are the specifications the <variable id="deliverable" value="%22Widgets%22"/> must meet for the <variable id="shipper" value="%22Dan%20Selman%22"/> to comply with its requirements and obligations under this agreement, detailed in <variable id="attachment" value="%22Attachment%20X%22"/>, attached to this agreement.
`\`\`\

[jeromesimeon]

This is definitely due to the last item with no content, which I think yields a list item with no child nodes

Most likely the new code has to be made more robust to the presence or absence of child nodes.

[jeromesimeon]

Easy way to reproduce the issue with markus:

bash-3.2$ cat test/data/nested-list.md 
Prolog
1. item one
2. item two
   - sublist
   - sublist
with some text in the same paragraph
   - 

Epilog
bash-3.2$ ../markdown-cli/index.js normalize --sample test/data/nested-list.md 
16:26:49 - info: 
Prolog
1. item one
2. item two
   - sublist
   - sublist
with some text in the same paragraph
   - 

Epilog
bash-3.2$ ../markdown-cli/index.js normalize --sample test/data/nested-list.md --slate 
16:26:52 - error: Node Item doesn't have any children!

[jeromesimeon]

So a little more investigation here. I think there is a bit of a discrepancy between markdown-slate and markdown-common.

1. The Concerto model for CommonMark uses an optional array for children:
   https://github.com/accordproject/models/blob/b209abc225db59fcf659b0445bf8723812fdbd2b/src/markdown/commonmark.cto#L23

2. markdown-common only creates a list of children when there is a need (so the default is no array, not empty array). I think this is where the logic is:
   

   markdown-transform/packages/markdown-common/src/CommonMarkTransformer.js

   Line 190 in bcf6330

   head.nodes = [];

   
   which only adds a nodes field in the enclosing head when there is at least one children to be added.

3. markdown-slate on the other hand, assumes there must be children nodes, and will treat the absence of one as an error. Here:

markdown-transform/packages/markdown-slate/src/ToSlateVisitor.js

Line 112 in bcf6330

if(!thing.nodes) {

Possible solutions

I think this can easily be fixed, but not sure what's the preference.

• We could keep markdown-common as is, but change markdown-slate so that absence of children is treated as an empty list of children.
• We could keep markdown-slate as is, but change markdown-common to create an empty array of children when there are no children. (maybe fixing the model too so it isn't optional).

@dselman Any opinion on this?

[jeromesimeon]

Possible solutions

I think this can easily be fixed, but not sure what's the preference.

• We could keep markdown-common as is, but change markdown-slate so that absence of children is treated as an empty list of children.
• We could keep markdown-slate as is, but change markdown-common to create an empty array of children when there are no children. (maybe fixing the model too so it isn't optional).

Note. I've experimented with the second option and it does fix the issue reported by @irmerk. (and of course breaks all the test snapshots in markdown-common which will now need to have nodes: [] added where necessary.

bash-3.2$ ../markdown-cli/index.js normalize --sample test/data/nested-list.md --slate 
16:39:05 - info: 
Prolog
1. item one
2. item two
   - sublist
   - sublist
with some text in the same paragraph
   - 

Epilog

[jeromesimeon]

Possible solutions

I think this can easily be fixed, but not sure what's the preference.

• We could keep markdown-common as is, but change markdown-slate so that absence of children is treated as an empty list of children.
• We could keep markdown-slate as is, but change markdown-common to create an empty array of children when there are no children. (maybe fixing the model too so it isn't optional).

@dselman Any opinion on this?

I think first option is less intrusive. Unless objections, I'll go ahead with that.

[dselman]

I agree -- option 1 sounds better.

[jeromesimeon]

Fixed in #127