search

vernak2539 / astro-rehype-relative-markdown-links

Rehype Plugin for Astro that allows for usage of relative links between markdown files
https://www.npmjs.com/package/astro-rehype-relative-markdown-links
15 stars 4 forks source link

bug: transforms `href` of non-anchor elements #33

Closed techfg closed 7 months ago

techfg commented 7 months ago

Per the README, I believe the intended behavior is only to transform links on anchor elements identified in markdown with the following:

[relative link](./other-markdown.md)

Currently, the plugin will transform any element that is a direct child of the root that contains an href property if the href meets the other required criteria (e.g., relative path, file exists, etc.).

Repro: https://github.com/techfg/astro-rehype-relative-markdown-links/tree/fix/only-transform-anchor-elements

Steps to reproduce:

  1. Clone https://github.com/techfg/astro-rehype-relative-markdown-links/tree/fix/only-transform-anchor-elements
  2. npm install
  3. Change to commit b58da46
  4. npm run test

Expected Result: Test should pass and the href on the div should not be transformed

Actual Result: The href is transformed

Additional Information

Difficult to create a live repro of this because any raw html (e.g., <div>foo</div> in a markdown will become a child element of another element.

vernak2539 commented 7 months ago

Thanks for this (and the PR). I'm unsure if this should be implemented. Here's my thoughts as to why:

  1. People shouldn't be putting hrefs on elements that don't have them (i.e. <div/> doesn't have an href attribute)
  2. If people are using the HTML elements that have them (i.e. <area/> and <base/>) then I feel we should transform them. I don't think anyone would use <base/> inside of a content page. They may use <area/> thought, which seems to be valid use case
  3. If people are linking to markdown using paths, they likely expect it to be transformed whether it's HTML in markdown or markdown

Thoughts?

techfg commented 7 months ago

People shouldn't be putting hrefs on elements that don't have them (i.e.

doesn't have an href attribute)

Famous last words! lol

In all seriousness, understand your points, all valid, hadn't thought about the area one in particular, definitely a valid use case. Can't see how base would ever have a scenario that would want transformed like you said.

One of my concerns currently is that the library is pretty broad and there are so many possible scenarios for resolution/evaluation. Without internal Astro information on things (btw, something to consider is taking a dependency on Astro to use some of the APIs & types but that's a different conversation), we have to do a lot of guessing/assuming, hence the options that have been added over time (e.g., basePath, contentPath, collectionPathMode). In reviewing other rehype "href" type of packages, some process anchor elements only (e.g., rehype-external-links) while others are broad (e.g., rehype-urls). It really comes down to use cases.

Kind of along the lines of #24, #26, #27 & #35, defining a baseline of functionality would be cool as currently we keep asking ourselves, is this a bug or a feature :)

Getting back to this question - I think it would be cool to support any href, definitely use cases beyond a tags for sure. With that said, does it add unnecessary complexity to handle additional scenarios we don't have adequate information for or not (e.g., what if the user doesn't want it transformed, what if outside the collection, etc.)? I can't think of any new scenarios that processing all href's would introduce beyond the ones we already know and need to define/find solutions for (e.g., #24, #25, #26) so likely OK to not restrict.

With that said, does it make sense to narrow our surface area for now to reduce risk & simplify and expand if/when someone needs the additional functionality?

Lemme know your thoughts - it's your call either way :)

vernak2539 commented 7 months ago

I think you've convinced me. Also, it says "markdown" in the title of the package, not "all forms of content that may have links."

techfg commented 7 months ago

lol, wasn't intentionally trying to convince, just sharing my thoughts :)

I do think narrowing scope in general for what the library does makes sense given the amount of unknowns during processing. Makes sense to be intentional about adding functionality beyond the basic use case which I believe is intended to be a tags in a collection md/mdx file referencing another md/mdx file in the same collection (see #26).

Ironically, I opened #35 which is completely counter to all the thoughts I've shared here lol But it would be an intentional decision to support that scenario I guess lol

Look forward to your thoughts on #26 & #27 when you have time. Have a great weekend!