Throwing an error on missing selector in test harness trigger #320
Conversation
aciccarello
left a comment
aciccarello
left a comment
There was a problem hiding this comment.
Looks great! Probably should be considered a breaking change.
| } | ||
|
|
||
| if (!firstItem) { | ||
| throw new Error(`Cannot find node with selector ${selector}`); |
There was a problem hiding this comment.
I like this error message. It might be good to make this and the assertion template errors consistent.
There was a problem hiding this comment.
I don't disagree, but updating the message there is a bit more complicated. I've made the change though, we'll see how it goes 😄
| throw Error(`Cannot find node with selector ${selector}`); | ||
| } | ||
| if (!isWNode(node) && !isVNode(node)) { | ||
| throw Error('Cannot set or get on unknown node'); |
There was a problem hiding this comment.
Hmm I don’t think we should change this. The findOne is meant to be equivalent to querySelector and not throw. It also doesn’t make sense to throw a setting unknown node here given the function name?
There was a problem hiding this comment.
Hehe, yeah, I wasn't sure about these changes. The other option is i can pass the selector in guard so it can use it in the error message, but thats going to end up looking silly and be a bit of a pain every time you call it. Maybe i should just remove that last commit?
There was a problem hiding this comment.
Ha yeah I agree, maybe for now we just stick with the trigger change and we can raise a separate issue for making error messages more consistent (it might be that we use the same function in the harness and assertion template)
This reverts commit f1fb897.
3 participants
Type: bug / feature
The following has been addressed in the PR:
prettieras per the readme code style guidelinesDescription:
Instead of silently failing, throwing an error when a
h.triggerencounters a selector that doesn't return any nodes.Resolves #318