-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: main
Are you sure you want to change the base?
feat: add relative path support for transforming urls #52
Conversation
After creating this PR, I identified an issue related to transforming 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 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 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 Look forward to everyone's thoughts! |
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! |
@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. |
❗ 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 includecollectionRelative
andpathRelative
(in addition to the previous options offalse
andname
).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:
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 theslug
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:
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.basePath
- Not needed for relativecollectionBase
(previouslycollectionPathMode
) - Not needed for relativecontentPath
- Not needed for relative and removed in feat: Add srcDir & aligncollection base
on a per collection basis (remove contentPath + contentPathMode) #49collections
- Added in feat: Add srcDir & aligncollection base
on a per collection basis (remove contentPath + contentPathMode) #49 to support collection overrides of top-level options but neithername
orbase
are needed for relativecollectionRelative
,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 thinkpathRelative
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 whilepathRelative
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 collectiondocs
, the transformed url would be:"collectionRelative"
:../../guides/section/my-guide
"pathRelative"
:my-guide
no
and both relative & absolute transform is supported, then should the optioncollectionBase
be renamed topathTransformationMode
or something different thancollectionBase
because when performingrelative
transformation, there is no base so the namecollectionBase
doesn't make sense anymore.Look forward to hearing everyone's thoughts!
Additional Information
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 & aligncollection base
on a per collection basis (remove contentPath + contentPathMode) #49 is merged. They are focused on refactoring the work done in feat: Add srcDir & aligncollection base
on a per collection basis (remove contentPath + contentPathMode) #49 a bit and improving test coverage.collection base
on a per collection basis (remove contentPath + contentPathMode) #49