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

convert relative urls to absolute #165

Merged
merged 1 commit into from
May 16, 2023

Conversation

karthik2804
Copy link
Contributor

@rajatjindal
Copy link
Contributor

I am not 100% certain if this is the right fix. we should probably take a step back and see how other tools solve this. Given how common this is, we cannot be the only one running into this problem.

@karthik2804
Copy link
Contributor Author

@rajatjindal that is a good point. I will look around to see if there is a case of people running into similar issues and potential solutions.

On the other hand, I feel this is the most straightforward way to this issue as making links absolute will ensure the routing works as intended.

@tpmccallum
Copy link
Contributor

@rajatjindal and @karthik2804 is it possible to enforce the removal of a trailing slash upon page load i.e. the server renders the page and never allows a trailing slash /?

@itowlson
Copy link
Contributor

@rajatjindal Can you qualify why you feel this is not the right fix? Would you have any objections to it as an interim fix if it solves a pain point? Absolute URLs in the Markdown makes authoring a complete bear trap as links are either broken or go to the production version of a page instead of the local one with your edits.

@rajatjindal
Copy link
Contributor

sorry, my comment is not a blocking comment but just a suggestion to check how other tools might be handling this.

@itowlson
Copy link
Contributor

Awesome, thanks @rajatjindal!

@karthik2804 any further concerns are is this safe to ship?

@karthik2804
Copy link
Contributor Author

It just needs a formal review and it should be good to ship.

@itowlson
Copy link
Contributor

Thanks, though from our conversation elsewhere it sounds like it's not strictly needed after all, as long as authors avoid relative paths? But I'm happy to review anyway (not that I know anything about Bartholomew but eh).

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel qualified to review this at a detail level but I assume it's been tested in both local and deployed environments and if so then LGTM. Thanks!

@karthik2804 karthik2804 force-pushed the absolutize_relative_url branch from 8c5e077 to 2458797 Compare May 16, 2023 00:34
Signed-off-by: karthik Ganeshram <[email protected]>
Signed-off-by: karthik2804 <[email protected]>
@karthik2804 karthik2804 force-pushed the absolutize_relative_url branch from 2458797 to 0a8779d Compare May 16, 2023 00:40
@karthik2804 karthik2804 merged commit 9eae95a into fermyon:main May 16, 2023
@karthik2804 karthik2804 deleted the absolutize_relative_url branch May 16, 2023 02:33
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.

4 participants