Fixes to "staying on the same page functionality" #7350
Conversation
ilyagr
force-pushed
the
fixbug
branch
3 times, most recently
from
3980fa8
to
04bc421
Compare
|
Thanks for the PR. However, the next time, before taking on work, please first discuss this with us as part of an issue or discussion, because we can omit unnecessary work first talking about it. We have no interest in using mocha as a test runner, as all my other projects use Jasmine, as it comes with everything included. Additionally, I always store my tests in a separate folder (see iframe-worker). Please split the entire testing logic into a separate PR, so we can discuss how we want to set up the environment, because there currently is none. We cannot merge this PR as it contains changes to the project structure, as well as changes to the business logic. I hope for your understanding – after all, it's me that has to maintain it. |
There seems to be some miscommunication, this is exactly what #7338 is. If/when we merge that one, this PR will become much smaller and will only include changes to business logic. I tried to explain my my plans and motivation as best I could there, as well. I'm not sure I was fully successful in explaining the relevance of that PR. In any case, I'm happy to adjust (Update: an now am done adjusting) that PR to something that works for you, as best as I can. Thanks for the pointer to I certainly appreciate the time you put into maintaining this project. My hope is to ultimately minimize your maintenance burden for this particular feature, which is currently very awkward to test changes to as you said yourself. |
I tried to follow the suggestions from squidfunk#7350 (comment) as best I can. The suggested `iframe-worker` template uses Karma, but I didn't use it because 1) it's a complicated tool I don't understand and 2) it says it's deprecated and isn't even accepting bugfixes in https://github.com/karma-runner/karma Instead, I followed https://www.innoq.com/en/blog/2020/04/ts-jasmine-karma/#executingtestsonnode.js to set up Jasmine with ts-node, since that's all I need for this application. If you need something like Karma or whatever replaced it, you can always rework this later. OTOH, let me know if there's more I can do to make this easier for you to review/maintain. I'm trying to make the minimal changes to the project structure possible, so that it's easy to change anything about this setup later.
I tried to follow the suggestions from squidfunk#7350 (comment) as best I can. The suggested `iframe-worker` template uses Karma, but I didn't use it because 1) it's a complicated tool I don't understand and 2) it says it's deprecated and isn't even accepting bugfixes in https://github.com/karma-runner/karma Instead, I followed https://www.innoq.com/en/blog/2020/04/ts-jasmine-karma/#executingtestsonnode.js to set up Jasmine with ts-node, since that's all I need for this application. If you need something like Karma or whatever replaced it, you can always rework this later. OTOH, let me know if there's more I can do to make this easier for you to review/maintain. I'm trying to make the minimal changes to the project structure possible, so that it's easy to change anything about this setup later.
I tried to follow the suggestions from squidfunk#7350 (comment) as best I can. The suggested `iframe-worker` template uses Karma, but I didn't use it because 1) it's a complicated tool I don't understand and 2) it says it's deprecated and isn't even accepting bugfixes in https://github.com/karma-runner/karma Instead, I followed https://www.innoq.com/en/blog/2020/04/ts-jasmine-karma/#executingtestsonnode.js to set up Jasmine with ts-node, since that's all I need for this application. If you need something like Karma or whatever replaced it, you can always rework this later. OTOH, let me know if there's more I can do to make this easier for you to review/maintain. I'm trying to make the minimal changes to the project structure possible, so that it's easy to change anything about this setup later.
|
|
This PR has gone stale. The following changes have not yet been addressed:
Could you please remove the setup of the testing infrastructure from this PR (as discussed in my last comments)? Otherwise, we can't consider it for merging. Thank you for investing the time! |
|
I am a little confused. The idea is to merge the testing infrastructure (which has always been split off into the separate PR #7338, just as you suggested in your quote) first. Then, I'd rebase this PR and all the testing infrastracture commits would disappear from it (since they'd be merged as part of #7338). The reason that this PR is marked as a draft is that PR #7338 (or some variation of it, or some other testing setup more acceptable to you and can do the job; #7338 if merely the simplest setup I could come up with) isn't merged yet, so indeed this PR is not ready to be merged yet either. Do you prefer to do something else? For example, do you want to merge this functionality without tests? It's possible, but I'd find it hard to believe that everything works correctly without the tests. Moreover, then the functionality could again easily break because of some seemingly unrelated changes without any warning. |
|
I can also rebase both PRs on top of the new master if it helps. Let me know if you need it sooner, otherwise I'll do it when I get to it. I think #7338 can be reviewed regardless, since all the merge conflicts are in One issue is that this PR is based on an older version of #7338. I didn't bother updating it so far since it's not ready to be merged anyway before something is decided about #7338, and then I could rebase it once on the final version of that PR. Nonetheless, since you're looking at this PR, I'll update it if I get a chance. |
|
Finally, to give you a little roadmap, if you'd just like to see how this PR works, take a look at the commit named "Fixes to staying on the same page functionality", currently at bbdfbc5. The changes to tests in that commit together with the fact that other tests don't change show that it works correctly. (The state of the tests before that commit represents the current functionality; they are introduced in the no-op #7338) I tried pretty hard to make each commit have a single purpose and be understandable in isolation. Once some version of #7338 is in, this PR will consist more or less of that commit and the following one. The last commit just rebuilds |
…tching versions"
We will add unit tests for this functions in the next commit.
The function gets its own file because I was unable to get the test runner ("mocha") to work otherwise. See the child commit's description for more details.
… 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.
The new implementation should be very flexible. 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`. The tests should demonstrate correctness of the implementation.
I polyfilled `URL.parse` since many browsers don't support it yet, apparently. I'd like this function to never throw an exception, any error should lead to it simply returning `undefined`.
I tried to follow the suggestions from squidfunk#7350 (comment) as best I can. The suggested `iframe-worker` template uses Karma, but I didn't use it because 1) it's a complicated tool I don't understand and 2) it says it's deprecated and isn't even accepting bugfixes in https://github.com/karma-runner/karma Instead, I followed https://www.innoq.com/en/blog/2020/04/ts-jasmine-karma/#executingtestsonnode.js to set up Jasmine with ts-node, since that's all I need for this application. If you need something like Karma or whatever replaced it, you can always rework this later. OTOH, let me know if there's more I can do to make this easier for you to review/maintain. I'm trying to make the minimal changes to the project structure possible, so that it's easy to change anything about this setup later.
|
We're not intending to merge the test infrastructure at this point, as I see several problems with how it is set up. To get this PR merged, please first entirely remove the testing infrastructure from this PR. We can then, at a later time, evaluate to also include the tests. The reason is that we currently have much, much bigger things on our plate, and I'm not looking to start with a testing infrastructure that I'm not happy with from the start. I currently also don't have the time to discuss ths in detail and hope for your understanding.
Well, all of Material for MkDocs is pretty stable without tests, but I absolutely agree that we should have some. If I'd start this project anew, I'd definitely include tests from the start. If you remove the tests from this PR, we can merge it. Otherwise, we'd need to close this without merge. I hope you can accept it for now. You know, priorities. |
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`.
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`.
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`.
|
Closing as stale / superseded. |
The new implementation should be very flexible.
Fixes #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 withmike serve.The tests should demonstrate correctness of the implementation.
This PR builds upon and includes #7338. The idea is that if you can be convinced
that #7338 is safe (it should be a clear no-op), the tests should be enough to convince you of the correctness
of the additional commits in this PR (they only change the
selectedVersionCorrespondingURLfunction), despite the significant change to the logic.I looked at #7227 and left a comment about how we could use that approach as well, but I think the approach here is much more flexible.
It's possible that there are more situations that we should test; let me know if you can think of some. For each significant case I'm aware of, I made a test based on exploring what happens in that case in a debugger.
I find it difficult to imagine a case when the old code will work and the new code won't, but it's possible to also keep the old logic in the function and guarantee that it will succeed whenever it succeeded before.
See commit descriptions for more details.