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
Migrate to JSDOM #1
Conversation
import Elmish.Test.Discover (childAt, children, find, findAll, findFirst, findNth) | ||
import Elmish.Test.Events (change, click, clickOn, fireEvent) | ||
import Elmish.Test.Query (attr, exists, find, findAll, findFirst, findNth, html, prop, tagName, text) | ||
import Elmish.Test.Query (attr, count, exists, html, prop, tagName, text) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I broke up the Query
module into Query
(reading properties of elements) and Discover
(manipulating/traversing the DOM tree). Initially it was necessary for my implementation of innerText
to avoid cyclic imports. I changed the implementation since then, but decided to leave the separation.
@@ -8,8 +8,6 @@ newtype DomProp (a :: Type) = DomProp String | |||
|
|||
value = DomProp "value" :: DomProp String | |||
|
|||
href = DomProp "href" :: DomProp String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out this is a not a property. Only accessible via getAttribute
.
count :: ∀ m. Testable m => String -> m Int | ||
count selector = length <$> findAll selector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out we're utilizing this in our codebase a lot, I had to convert all those places to findAll "..." >>= Array.length
, which is less readable. So I decided to add this utility here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sad-dom
🙁
But looks good and good to know we don’t need to load JSDOM before React 👍🏻
Sadly, looks like we have to move back to JSDOM, at least for now.
First there was the
oninput
kerfuffle, which can be monkey-patched, and now I also discovered that<button>
properties aren't being handler correctly, which is much harder to monkey-patch.It appears that there has been a two-month lull in
happy-dom
maintenance (last commit merged on July 24th, and there are 17 open PRs as of now), which raises a concern that the above issues might not get fixed soon.So, for now, we're moving back to JSDOM, and patching its deficiencies instead. So far I know of three problems:
innerText
- this looks like it could be reasonably replaced withtextContent
(see the list of differences on MDN), which is what I've done in this PR.window.scrollTo
, which crashes when called. This can be masked in the consuming codebase withwindow.scrollTo = () => {}
. I don't want to mask errors in the library.window.location.href
. Will have to be similarly patched.On the bright side, it doesn't look like JSDOM has to be registered before React is included after all. Just-in-time registration seems to work fine so far.