Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

question: expected behavior for links from/to locations not in a current content collection #26

Open
techfg opened this issue Apr 11, 2024 · 5 comments

Comments

@techfg
Copy link
Contributor

techfg commented Apr 11, 2024

Currently, the plugin runs against ANY markdown file, even if the file exists outside of a content collection. However, there is logic in the plugin that assumes the file its processing is within a content collection. This yields three scenarios that could lead to issues:

  1. Files within a content collection can reference files outside of its own content collection but within contentDir in another content collection. For example, src/content/posts/post-1.md could contain a markdown link to ../newsletter/post-1.md (a different content collection). Currently, this transformation works IF the actual page path for the site to the newsletter post-1 page includes the physical path name of newsletter (e.g., /newsletter/post-1).

  2. Files within a content collection can reference files outside of the content directory entirely. For example, src/content/posts/post-1.md could contain a markdown link to ../../pages/page-1.md. Currently, this transformation fails because it transforms to /../pages/page-1 which is not a valid path.

  3. Files outside of the content collection can reference files inside or outside of a content collection. For example, /pages/page-1.md could contain a markdown link to ../content/posts/post-1.md. This scenario will fail because the transformed url will become /pages/post-1

I think there is something to be said for allowing the portability of file references to markdown files in/out/across pages, content collections, etc., however the current logic does not support it but does actually do a transformation.

The question on the table is what scenarios to support for file references to {.md,.mdx} files:

  1. In/Around a single content collection only
  2. In/Around any content collection within contentDir
  3. Anywhere and everywhere

Repro: https://stackblitz.com/edit/github-pdzrjo-qqdlex

In the repro, there are two "Link" sections on each page to help navigate since a lot of links do not work because of the attempted transformations described above.

MD Links - This section always points to the corresponding .md file (e.g., ./page-1.md)
Explicit Links - This section always points to the site relative root page (e.g., /page-1)

Steps to reproduce:

  1. Open repro
  2. In the MD Links section, click any of the links starting with Root or Subdir

Expected Result:
This is the question since this page is /pages/index.md and not in a content collection

Actual Result:
Transformed url is based at /pages and all 404

  1. Click Newsletter 1 or Post 1

Expected Result:
That is the question

Actual Result:
Because of the physical directory structure is the same as the site url structure for the collection, the links actually work but if the directory structures were different, they would not

  1. Bounce around the pages across /pages/* and /newsletter/* and /posts/*. They all contain the same set of links separated with MD Links and Explicit Links. All the explicit links work but only some of the MD Links work based on physical directory structure, etc.

My thought is that for v1, only links in/around a single content collection should be supported and everything else should fail the IsValidRelativeLink test. This would be consistent with how Astro treats content collections generally speaking.

See #27 as well since its somewhat related although still separate and distinct.

Thoughts?

@techfg techfg changed the title bug (or intended): links outside of content collections are transformed question: expected behavior for links from/to locations not in the current content collection Apr 28, 2024
@techfg techfg changed the title question: expected behavior for links from/to locations not in the current content collection question: expected behavior for links from/to locations not in a current content collection Apr 28, 2024
@techfg
Copy link
Contributor Author

techfg commented Apr 28, 2024

Changed this issue from "bug" to "question" after creating issue #57 which covers the specific situation regarding links to markdown outside of a content collection since current behavior to those will result in incorrect links being transformed.

The scenario and questions raised in this issue's OP still stands, however. Assuming PR #49 is merged, and preferrably #50 is merged and #53 implemented, links from anywhere to a markdown file within a content collection directory should be reliable. This means that question 1 & part of question 3 (outside collection dir linking to inside a collection dir) in this issue's OP are addressed - again, relies on at least #49 being merged to cover basic scenarios while PR #50 & addressing Issue #53 would provide the flexibility needed to support all known scenarios.

Repositioning this issue to clearly define expected behavior in all scenarios so that any remaining issues can be fixed and/or required gaps/features backlogged.

The questions are, what should occur in the following scenarios:

  1. Markdown file in content collection directory links to a markdown file inside the same content collection directory (currently supported)
  2. Markdown file in content collection directory links to markdown file in another content collection directory (unofficially supported but could become official with feat: Add srcDir & align collection base on a per collection basis (remove contentPath + contentPathMode) #49 and preferably feat: add support for specifying collection names used in transformed URL paths #50 & feat: add support for dynamically resolving collection name and/or url path #53)
  3. Markdown file in a content collection directory links to a markdown file outside of a content collection directory (would require a new feature)
  4. Markdown file outside of a content collection directory links to a markdown file inside a content collection directory (unofficially supported but could become official with feat: Add srcDir & align collection base on a per collection basis (remove contentPath + contentPathMode) #49 and preferably feat: add support for specifying collection names used in transformed URL paths #50 & feat: add support for dynamically resolving collection name and/or url path #53)
  5. Markdown file outside of a content collection directory links to a markdown file outside a content collection directory (would require a new feature)

@techfg
Copy link
Contributor Author

techfg commented Dec 13, 2024

@vernak2539 -

I think reaching a decision on this one, especially given Astro v5 allowing content collections to live anywhere (see #74) is becoming more pressing.

My thoughts, for now, are that the plugin only resolve relative references within the "current" collection directory. Any relative links to another collection or anywhere else would be skipped. Currently, #58 ensured that the link must be in a content collection directory but I think we take it step further and restrict to the "current" content collection directory.

Although it could be done and likely done reliably, I think there are just too many possibilities and until there is stability with v5 and to limit potential issues, I think we still to "current collection" only. This would mean the answers to the questions I posed in my last comment are:

  1. Supported
  2. Not Supported
  3. Not Supported
  4. Not Supported
  5. Not Supported

Let me know your thoughts.

@vernak2539
Copy link
Owner

Sorry! I'll try to look at this one when I can (I've gotten through a few others though!)

@vernak2539
Copy link
Owner

I think reaching a decision on this one, especially given Astro v5 allowing content collections to live anywhere (see #74) is becoming more pressing.

My thoughts, for now, are that the plugin only resolve relative references within the "current" collection directory. Any relative links to another collection or anywhere else would be skipped. Currently, #58 ensured that the link must be in a content collection directory but I think we take it step further and restrict to the "current" content collection directory.

Although it could be done and likely done reliably, I think there are just too many possibilities and until there is stability with v5 and to limit potential issues, I think we still to "current collection" only. This would mean the answers to the questions I posed in my last comment are:

  1. Supported
  2. Not Supported
  3. Not Supported
  4. Not Supported
  5. Not Supported

Let me know your thoughts.

@techfg I think this makes a whole lot of sense. I like the idea of limiting it to the current directory, which both solves the issues outlined, albeit by limiting the functionality (not that it worked well/at all before), and let's us see how Content Layer pans out in v5+.

I like how this does still leave the door open for making things better in the future if there's a good way to do it

@techfg
Copy link
Contributor Author

techfg commented Dec 27, 2024

Agreed, just noting that we're limiting to current collection (not directory 😄). I think this greatly simplifies things and allows us to make the plugin much more reliable. In <= v4, cross collection links "work" but only when the files on disk match 1:1 to the page path. If we constrain to same collection only, coupled with the potential config/options changes for #75 (e.g., instead of srcDir/content, have each collection require an option configure by path instead of collection name), it should tighten things up. Once stable, as you mention, can always expand functionality.

One thing to note is that if we do change the options to require an entry per collection by path on disk instead of just collection name, we could continue to support cross collection links assuming the CollectionConfig for each collection contains its corresponding page path. This would create feature parity with current functionality recognizing that a collection could only have one page path. In practice, a collection could have more than 1 page path but we don't support this currently. The path property on CollectionConfig could be a function instead of a string which would allow for support to map a single collection to multiple page paths. This would be pretty cool I think but may be overkill at this stage. The config would look something like:

export type ResolvePath = (currentFilePath: string, targetFilePath: string) => URL;
type CollectionConfig = {
    base: "name" | false
    name?: string | ResolvePath
}

export type CollectionPath = string; // relative path from `srcDir` to content collection directory
type Options = {
    ...
    collections: Record<CollectionPath, CollectionConfig>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants