Rework lists

[wooorm]

Hi folks! 👋

After some thinking about lists, such as syntax-tree/mdast-util-to-hast#23 and #349, and the changes in syntax-tree/mdast#24, I reworked the lists as they are currently handled by remark-parse and remark-stringify. Below you can see the changes, but the most importent one is that I propose to remove loose on list and on listItem, instead replacing them by spread.

This PR additionally closes GH-350, GH-332, and GH-358.

/cc Could you review this @Hamms, as you spend some time thinking about this as well, and @ikatyang, as this will affect prettier?

Spread

So, previously we had loose. Any list item that was followed by a blank line between in and its next sibling, or if it had a blank line between any of its children, would result in that list item to receive loose: true, and otherwise loose: false.
A list would have loose: true if any of its children had loose: true, and loose: false otherwise.

This was based on the loose definition of CommonMark, which means it is meant for HTML (namely, in a loose list, all <p> tags are output, whereas in a tight list they are not).

This was okay, but wasn’t really useful for either markdown (remark-stringify) or html (mdast-util-to-hast), as both need different mechanisms (remark needs to know where to place blank lines, in the list, and in the items; mdast-util-to-hast needs to know whether to output <p> tags).

In this new solution, we break from the commonmark definition, by introducing a new term:

• Lists have spread: true if there is a blank line between any of its children (not in them), in which case remark-stringify will join all children with blank lines between them
• Items have spread: true if there is a blank line between any of its children (not in them), in which case remark-stringify will join all children with blank lines between them

This decouples the two, which makes for a simpler AST, but it comes with the downside that mdast-util-to-hast, when stringifying an item, needs to calculate whether its parent list or any of its siblings has spread: true, to see whether it needs to generate <p> tags.

This is good though, because the tree stable. Say we injected a new item into a list, that would previously create an unstable tree (stringifying it and parsing it again, would result in a different tree).

Changes

See c600489.

[ikatyang]

This looks great! 👍

But I think we still need the old loose field (maybe added by another plugin) to identify if two ASTs are the same from the perspective of CommonMark.

[wooorm]

@ikatyang Of course! I think we can do it as some tiny utility!

[Hamms]

I love the idea of decoupling the two, and it makes sense that that would be necessary to best serve both sets of interests.

It does seem - if I am correctly understanding this change - that this isn't really breaking from the commonmark definition in any significant way, just changing the time at which the definition is applied. We still apply the same rules to determine whether items or lists are loose or tight, and when rendering to HTML a list's tightness will still be determined by either its own tightness or that of its children, just now that is being determined when we consume the AST rather than when we generate it. Is that correct?

If so, it's not clear to me that it's best to rename the field from loose to spread; can you explain a bit more what your thinking is there?

[wooorm]

@Hamms Thanks for reviewing!

I love the idea of decoupling the two, and it makes sense that that would be necessary to best serve both sets of interests.

Perfect!

If so, it's not clear to me that it's best to rename the field from loose to spread; can you explain a bit more what your thinking is there?

I meant a break away from commonmark terminology, and I think if these properties do not really do what CommonMark calls “loose”, then it makes more sense to have a different word for it!

[Hamms]

if these properties do not really do what CommonMark calls “loose”, then it makes more sense to have a different word for it!

Agreed! I'm just having a hard time convincing myself one way or the other whether or not this is still doing what commonmark calls "loose" 🙃

A list is loose if any of its constituent list items are separated by blank lines, or if any of its constituent list items directly contain two block-level elements with a blank line between them.

In the current approach say that a list node is treated as being loose if it or any of its list items is labeled with spread: true. If we stick with the label loose over spread, then we say that a list node is treated as being loose if it or any of its list items is labeled with loose: true.

I think the thing I'm not clear on is whether we want a property on a node to imply that the node is that property, or that it's labeled with that property. I can see it being confusing if a node that is labeled with loose: false can be treated the same way a a node labeled with loose: true, depending on its children. But I can also see it being confusing that the property spread is used to determine a behavior known as loose.

Of course, either way I think this rework is great! I'm interested in the terminology change because the distinction seems subtle, but that also means it will likely be fine either way.

[wooorm]

Agreed! I'm just having a hard time convincing myself one way or the other whether or not this is still doing what commonmark calls "loose" 🙃

It’s doing different things but it could be mapped to the same looseness definition!

In the current approach say that a list node is treated as being loose if it or any of its list items is labeled with spread: true. If we stick with the label loose over spread, then we say that a list node is treated as being loose if it or any of its list items is labeled with loose: true.

Currently loose on a list is a combination of its own looseness and that of its children, potentially creating that inconsistent tree. With this change, it or it’s children could have spread, and its looseness could be calculated from that, always being consistent.

[hessj1]

@ikatyang Of course! I think we can do it as some tiny utility!

Is there a utility to allow for loose I am having issue after upgrading from v4 to 5@latest with my lists not being parsed into list items. like it used to.