-
Notifications
You must be signed in to change notification settings - Fork 68
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
🔗 Use READTHEDOCS_CANONICAL_URL
as baseurl if defined
#996
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great - happy to get this in fast! I left a question about the canonical url, and changing that to a pathname (and maybe testing it and raising helpful feedback).
} else if ((baseurl = process.env.READTHEDOCS_CANONICAL_URL)) { | ||
session.log.info('Building inside a ReadTheDocs environment'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is the full URL, and not just a part of it? (based on docs here).
I think that we should validate and only be taking the path part of the URL. in this case something like /en/latest
. Perhaps something like:
} else if ((baseurl = process.env.READTHEDOCS_CANONICAL_URL)) { | |
session.log.info('Building inside a ReadTheDocs environment'); | |
} else if (process.env.READTHEDOCS_CANONICAL_URL) { | |
baseurl = new URL(process.env.READTHEDOCS_CANONICAL_URL).pathname.replace(/\/$/, ''); | |
session.log.info(`Building inside a ReadTheDocs environment with BASE_URL="${baseurl}"`); |
The base URL is absolute and should not end with a trailing slash.
Now that there is a function, we could test the URL against that even a simple regexp like this: /^\/(.*)([^\/])$/
and raising an error if it doesn't match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kay, I've added the sanitization, with a bit different info reporting.
I was looking at adding a unit test, but I couldn't find a framework to use, and I'm not sure of how/where vitest
are defined
Signed-off-by: Cristian Le <[email protected]>
Is this one ready to go? Is it related to the other RTD PR? |
Yes, there were suggestions for adding unit test, but I couldn't figure out how to add them. Otherwise implementation wise it should be finished.
They are independent, but it would help simplify 1 line there. The RTD PR is a bit problematic due to the hydration and missing |
I can take another look later today, sorry for dropping the ball on getting these in! |
READTHEDOCS_CANONICAL_URL
as baseurl if definedREADTHEDOCS_CANONICAL_URL
as baseurl if defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a changeset, merging when tests pass!
There must be a better way around this with relative paths 1, but until then, this is a simple quick-fix for RTD deployments. The user would still need #984 to have a seamless experience.
Closes #983
Footnotes
https://github.com/executablebooks/mystmd/issues/188#issuecomment-2003171341 ↩