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

chore: avoid generating subdirectories for each page on new docs site #15967

Merged
merged 6 commits into from Jun 10, 2022

Conversation

mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jun 5, 2022

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 to foo/bar.html, which makes path foo/bar in the URL.

In the new docs site, Eleventy writes from foo/bar.md to foo/bar/index.html (this is default behavior), which makes path foo/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)

  • Added src/src.json that overrides default permalinks. This is the same as docs/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.
  • With this change, page.url includes the .html suffix, so I added prettyURL filter that removes .html, and used the filter in places where we're generating urls from page.url. Namely: docs index, sitemap, canonical links, and og:url.
  • With the above changes, links didn't work locally (http://localhost:2023), because browser-sync doesn't serve foo.html by default when foo 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-sync server settings, including settings related to pathPrefix, resulting in a totally broken site. As those settings look fairly complex, instead of trying to replicate them I decided to remove pathPrefix when the site is generated by npm start.
  • I also updated components library links. Is that okay?

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.

@mdjermanovic mdjermanovic added documentation Relates to ESLint's documentation chore This change is not user-facing labels Jun 5, 2022
@netlify
Copy link

netlify bot commented Jun 5, 2022

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 8f6793a
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/62a38105972497000831bb56
😎 Deploy Preview https://deploy-preview-15967--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@nzakas nzakas left a 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 => {
Copy link
Member

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.

Copy link
Member Author

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.

@mdjermanovic
Copy link
Member Author

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.

We are removing .html suffix in different places on the current website too: og:url, twitter:url, canonical url, blog post links, feed links. This is the same thing we do on the current website, I just extracted removing .html into a filter so that we don't have to repeat replace: '.html', ''.

@nzakas
Copy link
Member

nzakas commented Jun 8, 2022

I think we can actually solve this in a simpler way. Explained in #15844 (comment).

* see changes before deployed.
*/
const pathPrefix = process.env.CONTEXT === "deploy-preview" ? "" : "/docs";
const pathPrefix = ["local-serve", "deploy-preview"].includes(process.env.CONTEXT) ? "" : "/docs";
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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.

@@ -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",
Copy link
Member

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.

Copy link
Member

@nzakas nzakas left a 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.

@nzakas nzakas merged commit 90ff647 into main Jun 10, 2022
@nzakas nzakas deleted the issue15844-files branch June 10, 2022 18:43
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 22, 2022
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 ([#&#8203;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 ([#&#8203;15868](eslint/eslint#15868)) (Alex Bass)

#### Documentation

-   [`4871047`](eslint/eslint@4871047) docs: Update analytics, canonical URL, ads ([#&#8203;15996](eslint/eslint#15996)) (Nicholas C. Zakas)
-   [`cddad14`](eslint/eslint@cddad14) docs: Add correct/incorrect containers ([#&#8203;15998](eslint/eslint#15998)) (Nicholas C. Zakas)
-   [`b04bc6f`](eslint/eslint@b04bc6f) docs: Add rules meta info to rule pages ([#&#8203;15902](eslint/eslint#15902)) (Nicholas C. Zakas)
-   [`1324f10`](eslint/eslint@1324f10) docs: unify the wording referring to optional exception ([#&#8203;15893](eslint/eslint#15893)) (Abdelrahman Elkady)
-   [`ad54d02`](eslint/eslint@ad54d02) docs: add missing trailing slash to some internal links ([#&#8203;15991](eslint/eslint#15991)) (Milos Djermanovic)
-   [`df7768e`](eslint/eslint@df7768e) docs: Switch to version-relative URLs ([#&#8203;15978](eslint/eslint#15978)) (Nicholas C. Zakas)
-   [`21d6479`](eslint/eslint@21d6479) docs: change some absolute links to relative ([#&#8203;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 ([#&#8203;15986](eslint/eslint#15986)) (Strek)

#### Chores

-   [`f6e2e63`](eslint/eslint@f6e2e63) chore: fix 'replaced by' rule list ([#&#8203;16007](eslint/eslint#16007)) (Milos Djermanovic)
-   [`d94dc84`](eslint/eslint@d94dc84) chore: remove unused deprecation warnings ([#&#8203;15994](eslint/eslint#15994)) (Francesco Trotta)
-   [`cdcf11e`](eslint/eslint@cdcf11e) chore: fix versions link ([#&#8203;15995](eslint/eslint#15995)) (Milos Djermanovic)
-   [`d2a8715`](eslint/eslint@d2a8715) chore: add trailing slash to `pathPrefix` ([#&#8203;15993](eslint/eslint#15993)) (Milos Djermanovic)
-   [`58a1bf0`](eslint/eslint@58a1bf0) chore: tweak URL rewriting for local previews ([#&#8203;15992](eslint/eslint#15992)) (Milos Djermanovic)
-   [`80404d2`](eslint/eslint@80404d2) chore: remove docs deploy workflow ([#&#8203;15984](eslint/eslint#15984)) (Nicholas C. Zakas)
-   [`71bc750`](eslint/eslint@71bc750) chore: Set permissions for GitHub actions ([#&#8203;15971](eslint/eslint#15971)) (Naveen)
-   [`90ff647`](eslint/eslint@90ff647) chore: avoid generating subdirectories for each page on new docs site ([#&#8203;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>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 8, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants