Version switcher fails to stay on the same page when canonical_version is set in mike
#7226
Closed
4 tasks done
Labels
change request
Issue requests a new feature or improvement
resolved
Issue is resolved, yet unreleased if open
Comments
|
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 |
|
Thank you. I drafted a first idea of how this could be handled in #7227. Any feedback is appreciated |
|
Thanks for the PR! Please give me some time to check it out and give it a spin. |
This was referenced Jun 6, 2024
Closed
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.
|
I'm trying an alternative approach to fixing this in #7350. That approach should also make the version switcher work correctly with 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`.
|
Reopening until released. |
|
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
Context
We are using
mkdocs-materialtogether withmikefor our documentation.To improve SEO we set the
canonical_versioninmike. This sets therel="canonical"linkon every page and also leads to thesitemap.xmlin each version using the canonical links (withlatest). 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_versionyou 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-materialusing the sitemap of the version to determine if a sub page is available in that version. Since the sitemap always uses thecanonical_versioninstead 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
latestin allsitemap.xmlRelated
mikedocumentation and codesite_urlfromcanonical_versionReproduction in a fork from
mkdocs-material-example-versioningcanonical_version0.5(withcanonical_version). You can switch FROM this versions to others just fine0.4. Switch TO0.5and get redirected to the homepage0.5sitemap0.4sitemapReproduction
9.5.25-version-switcher-canonical.zip
Steps to reproduce
9.5.25-version-switcher-canonical.zipand navigate to the extracted folderpip install --upgrade --force-reinstall mike==2.1.1 mkdocs-material==9.5.25 mkdocs==1.6.0mkdocs.ymlremove the- infofrom thepluginssection and save the changesmike deploy canonicalmkdocs.ymlremove thecanonical_version: latestfrommikein thepluginssection and save the changesmike deploy no-canonical latestmike serveWorking as expected:
Not working as expected:
Browser
Chrome
Before submitting
The text was updated successfully, but these errors were encountered: