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.

Sign up for GitHub

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

Jump to bottom

Version switcher fails to stay on the same page when canonical_version is set in mike #7226

Open
4 tasks done
kevinh-hcl opened this issue May 31, 2024 · 4 comments · May be fixed by #7227 or #7350
Open
4 tasks done

Version switcher fails to stay on the same page when canonical_version is set in mike #7226

kevinh-hcl opened this issue May 31, 2024 · 4 comments · May be fixed by #7227 or #7350
Labels
change request Issue requests a new feature or improvement

Comments

@kevinh-hcl
Copy link

Context

We are using mkdocs-material together with mike for our documentation.

To improve SEO we set the canonical_version in mike. This sets the rel="canonical" link on every page and also leads to the sitemap.xml in each version using the canonical links (with latest). This seems to be the expected way of handling canonical URLs according to Google Search Central and others.

Bug description

When switching from a sub page of any version to a version that was built with the canonical_version you get redirected to the homepage of the version. The expected behaviour would be to stay on the same sub page, but in a different version.

From what I can see this is probably due to mkdocs-material using the sitemap of the version to determine if a sub page is available in that version. Since the sitemap always uses the canonical_version instead of the actual version, the page is not found and it falls back to the homepage.

Related links

Related documentation

Pointer from a previous discussion

Related mike documentation and code

Reproduction in a fork from mkdocs-material-example-versioning

Reproduction

9.5.25-version-switcher-canonical.zip

Steps to reproduce

  1. unzip 9.5.25-version-switcher-canonical.zip and navigate to the extracted folder
  2. (Optional: I had to remove the venv due to the size limitation in the .zip upload. Please follow https://squidfunk.github.io/mkdocs-material/guides/creating-a-reproduction/#environment to create a venv and activate it)
  3. Install the dependencies using pip install --upgrade --force-reinstall mike==2.1.1 mkdocs-material==9.5.25 mkdocs==1.6.0
  4. In mkdocs.yml remove the - info from the plugins section and save the changes
  5. Deploy a first version using mike deploy canonical
  6. In mkdocs.yml remove the canonical_version: latest from mike in the plugins section and save the changes
  7. Deploy a second version using mike deploy no-canonical latest
  8. Serve the documentation locally using mike serve

Working as expected:

  1. Open http://localhost:8000/canonical/subpage/
  2. Switch to the no-canonical version
  3. The version shows the correct page
  4. (Optional: check http://localhost:8000/no-canonical/sitemap.xml of the target version)

Not working as expected:

  1. Open http://localhost:8000/no-canonical/subpage/
  2. Switch to the canonical version
  3. Redirected to the homepage
  4. (Optional: check http://localhost:8000/canonical/sitemap.xml of the target version)

Browser

Chrome

Before submitting
@squidfunk
Copy link
Owner

Thanks for reporting and the very detailed explanations. Yes, it sounds like a currently unhandled combination of configuration settings when using mike. I'm not sure it's a bug, since we don't advertise the canonical_version setting anywhere in our documentation, so I'll tag this as an chance request. Since you have already invested some time into tracking down the cause, would you like to work on this in a PR? Happy to collaborate. Shouldn't be too hard to support ☺️

@squidfunk squidfunk added the change request Issue requests a new feature or improvement label May 31, 2024
@kevinh-hcl kevinh-hcl linked a pull request May 31, 2024 that will close this issue
Draft
@kevinh-hcl
Copy link
Author

Thank you. I drafted a first idea of how this could be handled in #7227. Any feedback is appreciated

@squidfunk
Copy link
Owner

Thanks for the PR! Please give me some time to check it out and give it a spin.

@squidfunk squidfunk linked a pull request Jun 1, 2024 that will close this issue
initial idea for canonical version logic #7227
Draft
This was referenced Jun 6, 2024
Closed
Closed
@ilyagr ilyagr mentioned this issue Jul 8, 2024
Open
ilyagr added a commit to ilyagr/mkdocs-material that referenced this issue Jul 9, 2024
@ilyagr
Add unit tests for "staying on the same page when switching versions"…
b5eeff8 
… functionality

This is meant to simplify checking correctness of code like squidfunk#7227 and fixing bugs like squidfunk#7226. In fact, I'm hoping to eventually make this code general enough that it runs locally with `mike serve`.

I picked `mocha` as the testing library because its Typescript support
relies on ts-node, which this project already relies on. I have little experience with `mocha` vs `jest` vs something else beyond this commit.

See also https://github.com/mochajs/mocha-examples/tree/main/packages/typescript

Instead of using `chai`, I'm using Node's assert package.
Its TS types are in `@types/node`. It should be trivial, and might be good, to switch to `chai` or something else.

This setup is not perfect, in particular I can't get tests to import the whole `index.ts` file. This is why the function being tested gets its own file. Importing `index.ts` would seem to require writing some DOM shims and be a general headache.

For the record, using `tsx` and `jsdom-global` instead of `ts-node` to run `mocha` seemed like the most promising approach, but it still failed since some files imported from this `index.ts` rely on the DOM having some particular structure in their top-level definitions.

Here's how to set up `tsx` and `jsdom-global`, for reference:

```diff
diff --git a/.mocharc.json b/.mocharc.json
index e713305556...33c9adc84d 100644
--- a/.mocharc.json
+++ b/.mocharc.json
@@ -2,5 +2,5 @@
     "$schema": "https://json.schemastore.org/mocharc.json",
     "extension": ["ts"],
     "spec": "src/**/**.test.ts",
-    "require": "ts-node/register"
+    "require": ["tsx", "jsdom-global/register"]
 }
diff --git a/package.json b/package.json
index ecc9f98cf9...10242ca725 100644
--- a/package.json
+++ b/package.json
@@ -76,6 +76,7 @@
     "gitlab": "^14.2.2",
     "google-fonts-complete": "jonathantneal/google-fonts-complete",
     "html-minifier": "^4.0.0",
+    "jsdom-global": "^3.0.2",
     "material-design-color": "^2.3.2",
     "material-shadows": "^3.0.1",
     "mocha": "^10.6.0",
@@ -99,6 +100,7 @@
     "svgo": "3.0.0",
     "tiny-glob": "^0.2.9",
     "ts-node": "^10.9.2",
+    "tsx": "^4.16.2",
     "typescript": "^5.5.2"
   },
   "engines": {
```

I can also incorporate the diff into this commit.
@ilyagr ilyagr linked a pull request Jul 9, 2024 that will close this issue
Draft
@ilyagr
Copy link
Contributor

ilyagr commented Jul 9, 2024 •
edited

I'm trying an alternative approach to fixing this in #7350. That approach should also make the version switcher work correctly with mike serve or if the website is published somewhere other than at site_url again.

I also set up some unit tests to stay sane while changing this behavior, hence the 6 commits.

ilyagr added a commit to ilyagr/mkdocs-material that referenced this issue Jul 11, 2024
@ilyagr
Add unit tests for "staying on the same page when switching versions"…
fd2194a 
… functionality

Following https://www.innoq.com/en/blog/2020/04/ts-jasmine-karma/#executingtestsonnode.js

This is meant to simplify checking correctness of code like squidfunk#7227 and fixing bugs like squidfunk#7226. In fact, I'm hoping to eventually make this code general enough that it runs locally with `mike serve`.

I picked `mocha` as the testing library because its Typescript support
relies on ts-node, which this project already relies on. I have little experience with `mocha` vs `jest` vs something else beyond this commit.

See also https://github.com/mochajs/mocha-examples/tree/main/packages/typescript

Instead of using `chai`, I'm using Node's assert package.
Its TS types are in `@types/node`. It should be trivial, and might be good, to switch to `chai` or something else.

This setup is not perfect, in particular I can't get tests to import the whole `index.ts` file. This is why the function being tested gets its own file. Importing `index.ts` would seem to require writing some DOM shims and be a general headache.

For the record, using `tsx` and `jsdom-global` instead of `ts-node` to run `mocha` seemed like the most promising approach, but it still failed since some files imported from this `index.ts` rely on the DOM having some particular structure in their top-level definitions.

Here's how to set up `tsx` and `jsdom-global`, for reference:

```diff
diff --git a/.mocharc.json b/.mocharc.json
index e713305556...33c9adc84d 100644
--- a/.mocharc.json
+++ b/.mocharc.json
@@ -2,5 +2,5 @@
     "$schema": "https://json.schemastore.org/mocharc.json",
     "extension": ["ts"],
     "spec": "src/**/**.test.ts",
-    "require": "ts-node/register"
+    "require": ["tsx", "jsdom-global/register"]
 }
diff --git a/package.json b/package.json
index ecc9f98cf9...10242ca725 100644
--- a/package.json
+++ b/package.json
@@ -76,6 +76,7 @@
     "gitlab": "^14.2.2",
     "google-fonts-complete": "jonathantneal/google-fonts-complete",
     "html-minifier": "^4.0.0",
+    "jsdom-global": "^3.0.2",
     "material-design-color": "^2.3.2",
     "material-shadows": "^3.0.1",
     "mocha": "^10.6.0",
@@ -99,6 +100,7 @@
     "svgo": "3.0.0",
     "tiny-glob": "^0.2.9",
     "ts-node": "^10.9.2",
+    "tsx": "^4.16.2",
     "typescript": "^5.5.2"
   },
   "engines": {
```

I can also incorporate the diff into this commit.
ilyagr added a commit to ilyagr/mkdocs-material that referenced this issue Jul 11, 2024
@ilyagr
Add unit tests for "staying on the same page when switching versions"…
d632ce2 
… functionality

Following https://www.innoq.com/en/blog/2020/04/ts-jasmine-karma/#executingtestsonnode.js

This is meant to simplify checking correctness of code like squidfunk#7227 and fixing bugs like squidfunk#7226. In fact, I'm hoping to eventually make this code general enough that it runs locally with `mike serve`.

I picked `mocha` as the testing library because its Typescript support
relies on ts-node, which this project already relies on. I have little experience with `mocha` vs `jest` vs something else beyond this commit.

See also https://github.com/mochajs/mocha-examples/tree/main/packages/typescript

Instead of using `chai`, I'm using Node's assert package.
Its TS types are in `@types/node`. It should be trivial, and might be good, to switch to `chai` or something else.

This setup is not perfect, in particular I can't get tests to import the whole `index.ts` file. This is why the function being tested gets its own file. Importing `index.ts` would seem to require writing some DOM shims and be a general headache.

For the record, using `tsx` and `jsdom-global` instead of `ts-node` to run `mocha` seemed like the most promising approach, but it still failed since some files imported from this `index.ts` rely on the DOM having some particular structure in their top-level definitions.

Here's how to set up `tsx` and `jsdom-global`, for reference:

```diff
diff --git a/.mocharc.json b/.mocharc.json
index e713305556...33c9adc84d 100644
--- a/.mocharc.json
+++ b/.mocharc.json
@@ -2,5 +2,5 @@
     "$schema": "https://json.schemastore.org/mocharc.json",
     "extension": ["ts"],
     "spec": "src/**/**.test.ts",
-    "require": "ts-node/register"
+    "require": ["tsx", "jsdom-global/register"]
 }
diff --git a/package.json b/package.json
index ecc9f98cf9...10242ca725 100644
--- a/package.json
+++ b/package.json
@@ -76,6 +76,7 @@
     "gitlab": "^14.2.2",
     "google-fonts-complete": "jonathantneal/google-fonts-complete",
     "html-minifier": "^4.0.0",
+    "jsdom-global": "^3.0.2",
     "material-design-color": "^2.3.2",
     "material-shadows": "^3.0.1",
     "mocha": "^10.6.0",
@@ -99,6 +100,7 @@
     "svgo": "3.0.0",
     "tiny-glob": "^0.2.9",
     "ts-node": "^10.9.2",
+    "tsx": "^4.16.2",
     "typescript": "^5.5.2"
   },
   "engines": {
```

I can also incorporate the diff into this commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
change request Issue requests a new feature or improvement
Projects
None yet
Milestone
No milestone
4 participants
@squidfunk @ilyagr @kevinh-hcl and others