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

initial idea for canonical version logic #7227

Conversation

kevinh-hcl
Copy link

Initial quick draft of an idea regarding #7226. This is a first draft, so feel free to give any feedback on improvements or misuse of any of the mechanisms of mkdocs or the theme.

@squidfunk
Copy link
Owner

Thanks for the PR. Could I ask you to simplify the control flow and scope the code added to a check for canonical_version? I'm not sure if this breaks anything, because versioning with mike is so hard to test, so I'd rather opt for an approach where we change behavior based on the presence of this flag without mixing with other code. It's currently too hard to follow with checkPath etc.

ilyagr added a commit to ilyagr/mkdocs-material that referenced this pull request 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 added a commit to ilyagr/mkdocs-material that referenced this pull request 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 pull request 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.
@squidfunk
Copy link
Owner

Superseded by #7559.

@squidfunk squidfunk closed this Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version switcher fails to stay on the same page when canonical_version is set in mike
2 participants