fix: page router fixed and tests added - #263 #271
Conversation
There was a problem hiding this comment.
I just skimmed over the PR, and have not really read all of it, so just some small notes.
I would recommend you to use the proper keywords for linking, so the issue gets closed when the related pr gets merged: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests
|
I think the choice of the DOM testing library is a big one and should fit to every test we do now and in the future. For example the whole Nue JS (reactive lib) should have a solid test suite that depends on a DOM library. Happy DOM might be good, but I'd like to explore more, especially since it seems to add 18MB of stuff to the node_modules folder. Something native to Bun would be amazing. |
mszmida
force-pushed
the
page-router-fix-issue-263
branch
from
7c480ee to
9ded123
Compare
|
@nobkd Thanks for the review! @tipiirai I fully agree with you. According to the Bun docs happy-dom is currently a recommended library for DOM testing purposes. The link that you provided is indeed very interesting however, its further fate is unknown. What I would like to point out is added value of this PR for the end users, as it actually fixes a known bug that is really annoying for anyone who wants to build anything with the usage of components, and also provides added value for the project devs by adding DOM tests for the fixed functionality, thanks to which the confidence of the code increases. The tests are fast and you can always switch to a better solution you find in the future. What do you think? Could you elaborate more what is a problem with the size of the |
Because one of the major goals of nue is to stay lean and "closer to metal" and dev dependencies are installed by default. I know there are |
|
But, we need some DOM testing utility for testing all the client-side stuff and I think the choice of the DOM library is an important one. Bun recommending Happy DOM puts a lot of weight so I guess that's the one to go with. Good find. I reviewed your code and it looks solid. Merging this now. btw: would you be interested in developing tests for the Nue JS reactive library? Thank you! |
|
@mszmida Awesome! So happy to hear that you want to contribute to Nue. I think one good starting point would be to write a single Nue JS test and make it pass. Currenly the Nue JS library is tested manually with these files. Would be awesome to have a clean/minimalistic/automated test runner for those. |
Fixes #263
I have also added tests for page router functionality that utilize https://github.com/capricorn86/happy-dom library to confirm that provided fix actually works. These tests have more integration nature to imitate human behavior as close as possible to make sure that the client code (in the browser) works as intended. I follow here Kent C. Dodds approach visible in his library https://github.com/testing-library/dom-testing-library. You probably know the guy but I didn't want to make any assumptions 😄
For tests purposes I created some helpers which you could use in other tests to cover more client code. Providing this solution I kept in mind to keep it minimal and as simple as possible.
Hope you will take a look into this PR because this bug is extremely annoying. I spotted also that components unmounting functionality is not implemented mentioned in #259. I could also help with that, but would like to know your ideas.
If you will have any questions just let me know. I am happy to help 😄