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

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

Closed
4 tasks done
kevinh-hcl opened this issue May 31, 2024 · 6 comments · Fixed by #7559
Closed
4 tasks done
Labels
change request Issue requests a new feature or improvement resolved Issue is resolved, yet unreleased if open

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
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
ilyagr added a commit to ilyagr/mkdocs-material that referenced this issue Jul 9, 2024
… 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
Copy link
Contributor

ilyagr commented Jul 9, 2024

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
… 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
… 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 Sep 24, 2024
This is squidfunk#7350 with the tests removed, as requested in
squidfunk#7350 (comment) .

I do urge you to reconsider and merge something like squidfunk#7350 (including
the tests I wrote) instead. Without tests, I suspect this code will
break like its [previous version did](
squidfunk@ceacfe5)
. Once the code does break, it might be difficult to fix without tests.

Even if other parts of the theme work without tests, and even if you aren't in love with
how I set up the infrastructure in squidfunk#7350 or squidfunk#7338, I think this logic is complex enough
to need tests to be maintainable. Again, I wouldn't be able to check this code's correctness
without the tests that aren't included in this PR. 

Of course, this is your project to maintain, so do what works best for you. I just hope
this bugfix helps.

---------------------

Fixes squidfunk#7226

Additionally, this allows using the version switcher even if the website
is published at a different URL than the `site_url`. In particular,
the version switcher should now work locally when serving the site with
`mike serve`.
ilyagr added a commit to ilyagr/mkdocs-material that referenced this issue Sep 24, 2024
This is squidfunk#7350 with the tests removed, as requested in
squidfunk#7350 (comment) .

I do urge you to reconsider and merge something like squidfunk#7350 (including
the tests I wrote) instead. Without tests, I suspect this code will
break like its [previous version did](
squidfunk@ceacfe5)
. Once the code does break, it might be difficult to fix without tests.

Even if other parts of the theme work without tests, and even if you aren't in love with
how I set up the infrastructure in squidfunk#7350 or squidfunk#7338, I think this logic is complex enough
to need tests to be maintainable. Again, I wouldn't be able to check this code's correctness
without the tests that aren't included in this PR. 

Of course, this is your project to maintain, so do what works best for you. I just hope
this bugfix helps.

---------------------

Fixes squidfunk#7226

Additionally, this allows using the version switcher even if the website
is published at a different URL than the `site_url`. In particular,
the version switcher should now work locally when serving the site with
`mike serve`.
ilyagr added a commit to ilyagr/mkdocs-material that referenced this issue Sep 24, 2024
This is squidfunk#7350 with the tests removed, as requested in
squidfunk#7350 (comment) .

I do urge you to reconsider and merge something like squidfunk#7350 (including
the tests I wrote) instead. Without tests, I suspect this code will
break like its [previous version did](
squidfunk@ceacfe5)
. Once the code does break, it might be difficult to fix without tests.

Even if other parts of the theme work without tests, and even if you aren't in love with
how I set up the infrastructure in squidfunk#7350 or squidfunk#7338, I think this logic is complex enough
to need tests to be maintainable. Again, I wouldn't be able to check this code's correctness
without the tests that aren't included in this PR. 

Of course, this is your project to maintain, so do what works best for you. I just hope
this bugfix helps.

---------------------

Fixes squidfunk#7226

Additionally, this allows using the version switcher even if the website
is published at a different URL than the `site_url`. In particular,
the version switcher should now work locally when serving the site with
`mike serve`.
@squidfunk
Copy link
Owner

Reopening until released.

@squidfunk squidfunk reopened this Sep 27, 2024
@squidfunk squidfunk added the resolved Issue is resolved, yet unreleased if open label Sep 27, 2024
@squidfunk
Copy link
Owner

Released as part of 9.5.39.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change request Issue requests a new feature or improvement resolved Issue is resolved, yet unreleased if open
Projects
None yet
4 participants
@squidfunk @ilyagr @kevinh-hcl and others