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

feat: add relative path support for transforming urls #52

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

techfg
Copy link
Contributor

@techfg techfg commented Apr 25, 2024

DO NOT MERGE YET

**EDIT** - My thoughts on this PR have shifted a bit since creating it with the below OP, see subsequent comments for current thinking.
 
This PR should be considered code complete, however there are a few points to be discussed and questions to be answered before merging. This PR builds on top of PR #49, #50 & #51, extending the collectionBase option to include collectionRelative and pathRelative (in addition to the previous options of false and name).

The goal of this PR is to add support for transforming relative markdown links to relative paths in the transformed url. Currently, the plugin transforms to absolute paths which has some shortcomings/limitations as discussed in #27. This PR addresses those limitations.

Transforming to relative URLs rather than absolute URLs has the following benefits:

  1. We do not need to know the site base path (if any)
  2. We do not need to know the collection name
  3. We do not need to know if the collection "lives" in the site root or in a "collection directory path"
  4. We can support a single content collection being served by multiple page paths (e.g., the content collection posts in ./src/content/posts could be served from page path /my-blog (e.g., ./src/pages/my-blog/[...slug].astro) and page path /your-blog (e.g., ./src/pages/your-blog/[...slug].astro).

When using relative paths, the only limitation that the plugin would have that I can think of is if getStaticPaths() dynamically determines the slug for a given entry in the collection that does not match the structure physically on disk AND there is no custom slug in the markdown file indicating the page path. This is a limitation that already exists in the plugin that there really is no way to solve for short of providing an option that allows the user to provide a function that dynamically resolves the path (will create a separate issue for this one - see #53).

The only potential downsides to using relative paths vs. absolute paths that I can think of would be any potential SEO or accessibility impact, however I do not believe relative paths are "scored" any differently than absolute paths (let me know if anyone knows otherwise) and I believe relative links are considered acceptable from an accessibility perspective (possibly not "as" accessible as absolute but still accessible).

Given all the above, the questions are:

  1. Should we add this PR and support relative path resolution?
  2. If yes to question 1 (which I strongly encourage 😄), should we eliminate the following options and support ONLY relative transformation? The benefit here is that relative addresses all the scenarios that require these options to exist and by eliminating them, it greatly reduces the maintenance cost of supporting the plugin.
    1. basePath - Not needed for relative
    2. collectionBase (previously collectionPathMode) - Not needed for relative
    3. contentPath - Not needed for relative and removed in feat: Add srcDir & align collection base on a per collection basis (remove contentPath + contentPathMode) #49
    4. collections - Added in feat: Add srcDir & align collection base on a per collection basis (remove contentPath + contentPathMode) #49 to support collection overrides of top-level options but neither name or base are needed for relative
  3. If yes to question 1 and regardless of answer to question 2, should we support collectionRelative, pathRelative or both? I've gone back and forth on this one as I can't see any benefit to one approach or the other so having both doesn't seem to make sense other than providing the user some flexibility (at the cost of plugin maintenance). If I were to lean one way, I think pathRelative makes the most sense as its the shortest distance from current to target and seems more "logical" when viewing the transformed url itself. For example, collectionRelative always navigates back up to the collection directory before then going back down to the target while pathRelative is a direct route (up/down/side). For example, given a file ./guides/section/my-guide.md referenced from ./guides/section/my-other-guide.md with the link [My Guide](./my-guide.md) in the content collection docs, the transformed url would be:
    • "collectionRelative": ../../guides/section/my-guide
    • "pathRelative": my-guide
  4. If the answer to question 2 is no and both relative & absolute transform is supported, then should the option collectionBase be renamed to pathTransformationMode or something different than collectionBase because when performing relative transformation, there is no base so the name collectionBase doesn't make sense anymore.

Look forward to hearing everyone's thoughts!

Additional Information

  1. There are some commits in this PR (and in feat: add support for specifying collection names used in transformed URL paths #50) that should be merged regardless of whether or not feat: add support for specifying collection names used in transformed URL paths #50 or this one is merged assuming feat: Add srcDir & align collection base on a per collection basis (remove contentPath + contentPathMode) #49 is merged. If the decision is to not merge feat: add support for specifying collection names used in transformed URL paths #50 or this one, I'll create a separate PR with those commits once feat: Add srcDir & align collection base on a per collection basis (remove contentPath + contentPathMode) #49 is merged. They are focused on refactoring the work done in feat: Add srcDir & align collection base on a per collection basis (remove contentPath + contentPathMode) #49 a bit and improving test coverage.
  2. This PR includes perf: cache frontmatter #51 but perf: cache frontmatter #51 can (and likely should assuming @vernak2539 agrees) be merged separately regardless of the decision on feat: Add srcDir & align collection base on a per collection basis (remove contentPath + contentPathMode) #49

@techfg
Copy link
Contributor Author

techfg commented Apr 28, 2024

After creating this PR, I identified an issue related to transforming index paths and have updated the PR to correctly handle them. In short, the challenge that index paths present when it comes to relative urls is that /docs/foo is different than /docs/foo/ in relative terms so when transforming links in a "current directory", it is critical that the current url be a directory.

As mentioned in the OP, transforming to relative paths instead of absolute paths has many benefits (e.g., eliminates a lot of the current required options, supports multiple page paths using the same content collection, etc.) with minimal setup/configuration by the user. With that said, it does require that the user ensures that any manually formed path in their site to an index location contain a trailing slash (e.g., /src/pages/my-page.astro would have to link to /docs/ and not simply /docs). This can be easily achieved by configuring trailingSlash="always" to the site and the plugin, however it's not ideal to require a specific trailingSlash configuration. Short of configuring trailingSlash, simply ensuring that all index paths contain a trailing slash in manual urls is easy enough to achieve but it doesn't stop a website visitor from navigating to an index path without specifying the trailling slash (in which case links to pages in current directory would not resolve correctly).

In short, my current thinking is that the answer to question 1 from OP is that relative paths have a lot of merit and flexibility, and we should strongly consider adding support for them, however my answer to question 2 is that we do eliminate the current set of options, only consider adding relative as an additional option (in which case for question 4, I would recommend we rename collectionBase option).

To be honest, I'm torn on whether or not to add relative support - many benefits, simplifies configuration, etc. but the downside is significant enough without careful attention by the user to make it such that possibly we should not add or at least not add currently.

I've created a full sample site using the latest code in this PR that contains various pathing scenarios. On the Site Index page, there are two links for My Blog Index and Your Blog Index, one with and one without an explicit trailing slash in the absolute link that is intended to highlight what occurs when trailing slash is not included in the MANUAL absolute link on the page.

Look forward to everyone's thoughts!

@vernak2539
Copy link
Owner

I've just merged #50 (and #49). @techfg after this, do you have any thoughts on this? I've not invested much time in gathering context, so wondering if I should devote any time to it.

@techfg
Copy link
Contributor Author

techfg commented Dec 5, 2024

@vernak2539 -

In re-thinking back through this one, I'm still torn on whether or not this change makes sense. In all but one specific scenario ("index" paths), changing to relative instead of absolute seems like a no-brainer as it makes things much simpler and aligns with how Astro itself works. However, it does introduce some risk requiring consumers of this plugin to ensure they properly identify an "index" path with a trailing slash so that the plugin can properly generate the correct path.

There hasn't been any input from the community on this so I don't think there is any urgency to get this merged. Given that, I do think it's worth you reviewing #27 and coming to a decision on that, then reviewing the comments I've made in this PR as I think it's ideal to get your perspective and input rather than me making a unilateral decision :)

All that said, as mentioned, I don't think there's any urgency so whenever you have the time to review. Feel free to share any questions/thoughts, happy to answer and then we can make a decision.

Thanks!

@techfg
Copy link
Contributor Author

techfg commented Dec 27, 2024

@vernak2539 - In thinking through this some more, I thought about i18n support. I haven't tested this yet but I don't think we currently support i18n in any way due to the way we resolve paths. In order to support i18n, we may need to strongly consider the concept of relative pathing instead of absolute pathing. There really is only one major gotcha with relative pathing as mentioned previously in this issue. We should consider i18n and this issue as decisions are finalized on #26 and #74.

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

Successfully merging this pull request may close these issues.

2 participants