Full reference links should have a fallback

[rubys]

Background: I'm converting node.js from marked to unified/remark/rehype, and found a regression. It seems that mdast-util-to-hast intentionally does not have a fallback for link references:

mdast-util-to-hast/test/link-reference.js

Lines 22 to 44 in f2e3ec5

	t.deepEqual(
	to(
	u(
	'linkReference',
	{
	identifier: 'delta',
	referenceType: 'full'
	},
	[u('text', 'echo')]
	)
	),
	u(
	'element',
	{
	tagName: 'a',
	properties: {
	href: ''
	}
	},
	[u('text', 'echo')]
	),
	'should not fall back on full `linkReference`s'
	)

This varies from the commonmark spec and other implementations:

https://spec.commonmark.org/0.28/#full-reference-link
https://spec.commonmark.org/dingus/?text=[foo][bar]

Is there a reason for this deviation?

[wooorm]

@rubys Sorry for the slow reply!

The reason that this isn’t represented in the AST is that it is based on the output. Markdown stringifies those differently than HTML.

However, if mdast-util-to-hast produces an AST that’s stringified differently than how commonmark/github does it, it’s a bug. It shouldn’t deviate, and I welcome a PR!

[rubys]

mdast-util-to-hast does indeed produce an AST that is stringified differently than how commonmark/github does it. You can quickly verify this by entering [foo][bar] in the commonmark dingus page.

A PR, with updated test cases can be found at #21

[wooorm]

Oh sorry, missed it!

[rubys]

Thanks! It looks like I will have a follow-on issue that will require a slightly bigger change (not so much in terms of lines of code, but affecting not only docs, but also a small change to remark-rehype).

The issue is that remark-parse's mdast tree for potential link or image reference is lossy... for example, the identifier is converted to lowercase.

The fix I suggest is that callers to mdast-util-to-hast can optionally pass the file.contents as an option. If that option is present, and position information is present, and an undefined reference is encountered, then the undefined reference will be replaced with the original text from the file contents. If any one of these conditions is not met, the current fallback logic will be used.

Thoughts?

[wooorm]

In fine discussing this! It does sound like a lot of logic, couldn't we perhaps add a new 'reference' property to references that define the value that was used, next to reference type?
The downside is that this could result in inconsistent trees, but maybe we could work around that somehow?

[rubys]

I don't understand the 'inconsistent trees' part, but looking into it, I agree that that is a better approach. Looking at the VFile content, even when augmented by position information, doesn't guarantee that the original source was markdown. Conceivably somebody could have a unified pipeline where there was some other source format which is subsequently converted first to markdown and then to HTML. Unlikely, but possible.

So the outline for the set of PRs I will be developing: first to MDAST itself describing the new property to all three of LinkReference, ImageReference, and FootnoteReference. Then there will be a PR for remark-parse to add this property in those three cases. And finally a PR to md-to-ast to look for this property and use it as the fallback if it is present, and if not, continue with the current logic.

Anything I'm missing?

[rubys]

Three pull requests ready for review:

• Add reference field to LinkReference, ImageReference mdast#23
• Add reference property on full references when different from identif… remarkjs/remark#346
• Prefer reference over identifier when reverting an unresolved reference. #22