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
chore: avoid generating subdirectories for each page on new docs site #15967
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Thanks for taking a look at this. This approach feels error-prone to me. If we didn’t need a special filter on the current website, that seems to indicate that there should be a way to do the same thing without a special filter that we need to remember to use in different places.
* `page.url` will include the `.html` suffix for all documents | ||
* except for those written as `index.html` (their `page.url` ends with a `/`). | ||
*/ | ||
eleventyConfig.addFilter("prettyURL", url => { |
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.
I’m concerned about this approach. I don’t think it’s at all clear where we should and should not be using this filter, and I fear this will cause more errors in the future.
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.
The rule would be to use this filter in all places where page.url
is written into the output content. In places where it is used in calculations, there is no need to use the filter (example on the current website). This is the same approach as on the current website.
If you have an idea for another approach, I could try it. We could maybe write the files without .html
extension, but that would probably require additional configuration on Netlify to treat extensionless files as html.
We are removing |
I think we can actually solve this in a simpler way. Explained in #15844 (comment). |
docs/.eleventy.js
Outdated
* see changes before deployed. | ||
*/ | ||
const pathPrefix = process.env.CONTEXT === "deploy-preview" ? "" : "/docs"; | ||
const pathPrefix = ["local-serve", "deploy-preview"].includes(process.env.CONTEXT) ? "" : "/docs"; |
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.
We should use the pathPrefix
locally too, otherwise, we won't be able to tell when the url
filter isn't used. The way I set it up was so that these errors would be obvious locally because we won't get them in deploy preview.
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.
I couldn't make it work (see the third point in "What changes did you make" in the original post). I'll see if the browser-sync config override can be done differently.
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.
Huh. I haven’t had any trouble since I first added pathPrefix. Eleventy generates an index.html file at the root that redirects to /docs and then everything works perfectly for me. I’m using the same setup on the playground.
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.
Eleventy's built-in server returns the content of foo/index.html
when we request foo/
, but it doesn't return the content of foo/bar.html
when we request foo/bar
, which is what we'll have after this change. It needs some additional configuration.
Without pathPrefix, it works well when I add serveStaticOptions: { extensions: [ 'html' ] }
to the config that eleventy passes into the underlying server.
eleventyConfig.setBrowserSyncConfig({
server: {
baseDir: "_site",
serveStaticOptions: {
extensions: ["html"]
}
}
});
With pathPrefix, for some reason it doesn't work.
eleventyConfig.setBrowserSyncConfig({
server: {
baseDir: "_site/_eleventy_redirect",
routes: {
"/docs": "_site"
},
serveStaticOptions: {
extensions: ["html"]
}
}
});
Cannot GET /docs/user-guide/configuring/configuration-files
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.
I tried with rewriting URLs instead of configuring the server. That seems to work well so I restored pathPrefix for local development.
docs/package.json
Outdated
@@ -14,7 +14,7 @@ | |||
"watch:eleventy": "eleventy --serve --port=2023", | |||
"build:sass": "sass --style=compressed src/assets/scss:src/assets/css --no-source-map", | |||
"build:eleventy": "npx @11ty/eleventy", | |||
"start": "npm-run-all build:sass --parallel watch:* --parallel images", | |||
"start": "cross-env CONTEXT=local-serve npm-run-all build:sass --parallel watch:* --parallel images", |
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.
As mentioned earlier, let's leave this as-is.
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.
LGTM. I verified this is working for me locally.
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.17.0` -> `8.18.0`](https://renovatebot.com/diffs/npm/eslint/8.17.0/8.18.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.18.0`](https://github.com/eslint/eslint/releases/tag/v8.18.0) [Compare Source](eslint/eslint@v8.17.0...v8.18.0) #### Features - [`a6273b8`](eslint/eslint@a6273b8) feat: account for rule creation time in performance reports ([#​15982](eslint/eslint#15982)) (Nitin Kumar) #### Bug Fixes - [`f364d47`](eslint/eslint@f364d47) fix: Make no-unused-vars treat for..of loops same as for..in loops ([#​15868](eslint/eslint#15868)) (Alex Bass) #### Documentation - [`4871047`](eslint/eslint@4871047) docs: Update analytics, canonical URL, ads ([#​15996](eslint/eslint#15996)) (Nicholas C. Zakas) - [`cddad14`](eslint/eslint@cddad14) docs: Add correct/incorrect containers ([#​15998](eslint/eslint#15998)) (Nicholas C. Zakas) - [`b04bc6f`](eslint/eslint@b04bc6f) docs: Add rules meta info to rule pages ([#​15902](eslint/eslint#15902)) (Nicholas C. Zakas) - [`1324f10`](eslint/eslint@1324f10) docs: unify the wording referring to optional exception ([#​15893](eslint/eslint#15893)) (Abdelrahman Elkady) - [`ad54d02`](eslint/eslint@ad54d02) docs: add missing trailing slash to some internal links ([#​15991](eslint/eslint#15991)) (Milos Djermanovic) - [`df7768e`](eslint/eslint@df7768e) docs: Switch to version-relative URLs ([#​15978](eslint/eslint#15978)) (Nicholas C. Zakas) - [`21d6479`](eslint/eslint@21d6479) docs: change some absolute links to relative ([#​15970](eslint/eslint#15970)) (Milos Djermanovic) - [`f31216a`](eslint/eslint@f31216a) docs: Update README team and sponsors (ESLint Jenkins) #### Build Related - [`ed49f15`](eslint/eslint@ed49f15) build: remove unwanted parallel and image-min for dev server ([#​15986](eslint/eslint#15986)) (Strek) #### Chores - [`f6e2e63`](eslint/eslint@f6e2e63) chore: fix 'replaced by' rule list ([#​16007](eslint/eslint#16007)) (Milos Djermanovic) - [`d94dc84`](eslint/eslint@d94dc84) chore: remove unused deprecation warnings ([#​15994](eslint/eslint#15994)) (Francesco Trotta) - [`cdcf11e`](eslint/eslint@cdcf11e) chore: fix versions link ([#​15995](eslint/eslint#15995)) (Milos Djermanovic) - [`d2a8715`](eslint/eslint@d2a8715) chore: add trailing slash to `pathPrefix` ([#​15993](eslint/eslint#15993)) (Milos Djermanovic) - [`58a1bf0`](eslint/eslint@58a1bf0) chore: tweak URL rewriting for local previews ([#​15992](eslint/eslint#15992)) (Milos Djermanovic) - [`80404d2`](eslint/eslint@80404d2) chore: remove docs deploy workflow ([#​15984](eslint/eslint#15984)) (Nicholas C. Zakas) - [`71bc750`](eslint/eslint@71bc750) chore: Set permissions for GitHub actions ([#​15971](eslint/eslint#15971)) (Naveen) - [`90ff647`](eslint/eslint@90ff647) chore: avoid generating subdirectories for each page on new docs site ([#​15967](eslint/eslint#15967)) (Milos Djermanovic) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1427 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:
Refs #15844
In the current site (eslint/website repo), Eleventy writes from
foo/bar.md
tofoo/bar.html
, which makes pathfoo/bar
in the URL.In the new docs site, Eleventy writes from
foo/bar.md
tofoo/bar/index.html
(this is default behavior), which makes pathfoo/bar/
in the URL.The added trailing slash breaks document-relative links.
This PR fixes the new docs site to generate files in the same way as the current site.
What changes did you make? (Give an overview)
src/src.json
that overrides default permalinks. This is the same asdocs/docs.json
on the current site. Per the data cascade, this shouldn't affect templates that have a permalink in the front matter (like the sitemap, robots, etc.), which is good.page.url
includes the.html
suffix, so I addedprettyURL
filter that removes.html
, and used the filter in places where we're generating urls frompage.url
. Namely: docs index, sitemap, canonical links, and og:url.http://localhost:2023
), because browser-sync doesn't servefoo.html
by default whenfoo
is requested, so I had to add"html"
extension in the browser-sync config through eleventy. Unfortunately, eleventy shallow merges the added config, so this change overwrites all eleventy's browser-syncserver
settings, including settings related topathPrefix
, resulting in a totally broken site. As those settings look fairly complex, instead of trying to replicate them I decided to removepathPrefix
when the site is generated bynpm start
.Is there anything you'd like reviewers to focus on?
Check that the links work well in the deployment preview, and that there are no trailing slash redirections.