Add reference field to LinkReference, ImageReference

[rubys]

No description provided.

[wooorm]

Would it make sense to drop the case where referenceType is full? Although it’s not needed to create the original markdown for collapsed or shortcut references in remark, it’s one less condition in the spec?

[wooorm]

Maybe it could be useful for linting, or non-HTML and non-markdown outputs?

[rubys]

Two thoughts:

1. I don't know how to trade-off the additional memory usage vs the overhead (conceptual and runtime) of a single if-check.

2. What I do know is that 50+ tests within the remark repository alone will need to be updated should the check for full and different than identifier be dropped. If there is that much change within the repository, chances are high this change will affect somebody's test suite out there.

As to usefulness, this comment was not meant to be an exhaustive list. I did provide a patch for remark-stringify, so this new property is arguably useful in the generation of markdown (i.e., not just the consumption).

[wooorm]

• For the trade-off, I think that’s fine. How many references are out there really.
• For the test suite: the remarkjs repo has a script: script/regenerate-fixtures that updates the ASTs for all tests, and you can check the diff if it works or not. I personally don’t see the addition of a property to nodes as a breaking change. Even if someones tests break because of fixtures, I don’t think actual (not sure how to call it) code will break.
• And usefulness: I meant that the addition of a reference to non-full references could potentially be useful for other things, like linting, even though we’re now discussing stringifying to markdown and html, which doesn’t need it.

[rubys]

If you are thinking about the linting case, shouldn't references also be captured for Footnote Reference, Definitions and FootnoteDefinition too?

[wooorm]

@rubys Darn, that’s true. If we’re adding a value that is the “original” version of identifier (which makes sense to me), we should use a different name though... Any suggestions?

[wooorm]

Some ideas, mostly bad ones, sorted from like to dislike:

• identifierRaw
• identifierSource
• reference
• location
• originalIdentifier
• id

[rubys]

I was going to suggest rawIndentifier which is essentially your first choice.

[wooorm]

Hmm. OK.

But now I’m wondering. We could add raw versions of most properties, e.g., they are mostly decoded to remove entities for example, so if we’re going the route of adding raw versions of computed properties, would an optional raw object on nodes make more sense? That could store the original versions of props, but also opening and closing markers, white-space, or similar?

I see a value in this, even though it’s far away before it’s implemented.
But I fear it would conflict if we add this property outside of that object now?

[rubys]

We appear to be wandering off into Yak Shaving territory. :-)

I don't think we need to worry to much about a linter application requirements. An application could safely get the original source via file.contents.slice(node.position.start.offset, node.position.end.offset).

We don't have that luxury in mdast_util_to_hast as (1) it doesn't currently have access to file, and (2) as a utility, it doesn't know whether the original source is in markdown format (i.e., there could have been a transformation to MDAST other than remark_parse.

My leaning are to not boil the ocean, and just focus on the requirements we know. After all, in all other cases than full for link and image references, the raw identifier can be determined just by looking at other MDAST properties.

[wooorm]

I know it’s yak shaving, but it’s also fair to think about it for a day!
However, what we require can be accomplished with a) .identifierRaw, and b) .raw.identifier.

a) is simple for now, but could stand in the way of .raw being added in the future!

[rubys]

@woorm: what's the verdict?

[wooorm]

I think I prefer reference. Even in relation to identifier (on non-reference) nodes it’s okay I believe.
That way it won’t conflict with any “raw” properties in the future.

What do you think? Big mistake?

For the implementation, I’m partial to whether only references, or all nodes with an identifier should receive these new props. Whatever works for you!

[rubys]

I believe I've addressed your feedback. Note: I went with definition for Defintion and FootnoteDefinition nodes.

If this looks correct overall, I'll update remarkjs/remark#346 to match. I don't believe that syntax-tree/mdast-util-to-hast#22 has to change.