Serialize Wrapper Instances #4
Conversation
index.js
Outdated
| @@ -1,11 +1,15 @@ | |||
| const beautify = require('pretty') | |||
|
|
|||
| const isHtmlString = received => typeof received === 'string' && received[0] === '<' | |||
| const isVueWrapper = received => typeof received === 'object' && received.isVueComponent | |||
There was a problem hiding this comment.
This property will soon be renamed to isVm. Instead use the public method isVueInstance()
There was a problem hiding this comment.
Got it, wasn't aware of that. Will do.
index.js
Outdated
| const isVueWrapper = received => ( | ||
| typeof received === 'object' && | ||
| received.isVueInstance && | ||
| received.isVueInstance() |
There was a problem hiding this comment.
@eddyerburgh I made the change as you suggested. I am wondering if you think it's necessary to actually call isVueInstance when the existence of the method is already Vue-specific. I put both the existence check and the call in for now, but will defer to your judgement.
|
Thanks for taking a look so quickly. I made the change as you suggested and squashed it back in. There's one question I had about your preferred usage, but other than that it should be good to go. |
index.js
Outdated
| const isHtmlString = received => typeof received === 'string' && received[0] === '<' | ||
| const isVueWrapper = received => ( | ||
| typeof received === 'object' && | ||
| typeof received.isVueInstance === 'function' && |
There was a problem hiding this comment.
@eddyerburgh I made the change as you suggested. I am wondering if you think it's necessary to actually call isVueInstance when the existence of the method is already Vue-specific. That is, of course, assuming this method will always return true. I put both the existence check and the call in for now, but will defer to your judgement.
(Repeating this comment due to the previous one being shown as outdated due to a squashed commit).
There was a problem hiding this comment.
Good point, the existence is enough 👍
There was a problem hiding this comment.
Great, thanks. I've pushed that change.
|
This has been released in v2.0.0 |
|
Awesome! I now realize that one change I forgot to make as part of this PR was updating the example in the README to reflect the new usage. |
Thanks for this project!
I stumbled upon this after a friend noticed hugely inflated snapshot sizes in his codebase, where developers were forgetting to call
.html()in their snapshot assertions.Digging into it, I noticed that the current implementation requires the user to call
.html()before passing the wrapper in. It seems like this should be the primary concern of the serializer, since the purpose of configuring a global serializer such as this is to remove the burden of manually doing it in every assertion. This is how Enzyme's popular toJSON serializer works, for example.This would likely be a major version upgrade since it would break snapshots for anybody that was inadvertently snapshotting entire instances.
Thanks in advance for taking a look :)