-
Notifications
You must be signed in to change notification settings - Fork 100
🎯 Render static HTML pages to expected server path #2178
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
🎯 Render static HTML pages to expected server path #2178
Conversation
Lol if it's as easy as this I will be pleasantly surprised. Also, is this the kind of thing that would break anything? If so we might want a feature flag for it (eg trailingslash, similar to docusaurus) and in a few releases switch the defaults |
I was worried it would break paths to JS bundles, but because we currently don't support relative paths, it doesn't. |
Here's my suggested path forward:
To me, this feels like it better aligns the HTML links behavior with other state of the art documentation generators, and would make it more likely to work across a number of hosting providers..so I'm for it! |
Opting in sounds great to me, this is currently the blocker for a few repos of mine that are otherwise ready to be switched to JB2. |
@stefanv this looks fine! We should check that any steps that rewrite the HTML still find the file. If that's the case, I'd also feature flag and merge this given that it unblocks quite a lot of useful testing. We don't have a standard FF system, so I'd suggest a MYST_ prefixed environment variable. @rowanc1 can take a look (I'm deliberately afk this week), but I think it's also important that these kind of things are safe to just ship. Let's include this (with FF) in a release this weekend? |
@@ -35,7 +35,7 @@ export async function currentSiteRoutes( | |||
const pageSlug = slugToUrl(page.slug); | |||
return { | |||
url: `${host}${projSlug}/${pageSlug}`, | |||
path: path.join(proj.slug ?? '', `${pageSlug}.html`), | |||
path: path.join(proj.slug ?? '', `${pageSlug}/index.html`), |
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.
So I think this could be something like this?
path: path.join(proj.slug ?? '', `${pageSlug}/index.html`), | |
// I'm not sure how to get options but I imagined it being under `site.options.trailingSlash` similar to docusaurus. | |
const trailingSlash = site.options?.trailingSlash === true; | |
const filePath = trailingSlash | |
? path.join(proj.slug ?? '', pageSlug, 'index.html') | |
: path.join(proj.slug ?? '', `${pageSlug}.html`); |
@agoose77 why not have this feature flag at the level of It feels similar to the |
We could do! We don't have a mechanism yet. I think we should distinguish between options and feature flags, as the latter is 'unstable'. To add - this probably should be a site option eventually, even if it is default on - it's more that we can ship right now imo if we don't expose any configuration changes. |
Ah got it -if our goal is just unblock experimentation here then I'm happy with whatever is simplest and doesn't change anything for existing users. If like a |
🦋 Changeset detectedLatest commit: 585c91d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
I pushed a little bit of docs to this to explain the behavior. I think this is pretty much ready to try-out no? If this unblocks users being able to use ReadTheDocs, that would be very helpful... |
Nice - tested locally as well, all looks good. There shouldn't be any impact on github pages, and then it opens up against RTD, which is awesome. |
Servers typically translate
foo/bar
tofoo/bar/index.html
. If we put the pages there, then the server finds them, and we don't need to jump through any further hoops to get them rendered.