-
Notifications
You must be signed in to change notification settings - Fork 668
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
fix(create-instance): revert stubbing of component _Ctor #1479
Conversation
I am also not 100% sure on how that part of the code base works. Since all the tests seem to be passing, I think this is fine. Let's see if @dobromir-hristov or @JessicaSachs has any thoughts, if not I'm happy to merge this up. |
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.
Seems ok... 🤔 let's get a second opinion.
I think this is a sort of cache that Edd cleared ... But I am not sure |
I say we go for it - it's breaking at least one test suite for no obvious reason, and our tests aren't catching it. |
Thanks for merging this fix! Any ETA for a tagged release? We're excited to upgrade! |
I can do a release this week! |
Just a note to maybe help others. After this change (patch-create-element.js#extend remove
This does work (also after #1411 hits it will be the default behavior):
|
@sullivanpt thanks for the feedback, this is useful to know about. The next release will be 1.0! Aiming for end of this month. All that is left is backport a few things from VTU next (mainly |
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
The PR fulfills these requirements:
dev
branch.fix #xxx[,#xxx]
, where "xxx" is the issue number) N/AOther information:
Hello and thank you for these awesome utils, they make testing components a breeze!
This PR is a fix to address what is likely to be a very extreme edge-case. It reverts three small changes made between v1.0.0-beta.29 and v1.0.0-beta.30 that are preventing us from being able to upgrade. The offending changes were included in this commit 0c07653, but appear to not specifically be a requirement for that change.
When upgrading v1.0.0-beta.30, a few tests for our custom scrollable table started to fail. After some careful investigation, it appears the cause of the failure was the inability to support the DOM restructuring done in the directive. We do some shallow cloning of Nodes and move some parts around using standard DOM APIs including
insertBefore
andappend
.I've tried to come up with the exact scenario to reproduce the issue, but I've come up short! Here's what we're doing in a nutshell:
<thead>
out and putting it in a new<table>
alongside the component's rendered table. This approach works perfectly in browsers and with test-utils v1.0.0-beta.29 in jest.I've attempted to recreate the scenario in a unit test, but I can't get the test to fail, so I'm confident I'm missing a key piece of knowledge around the inner workings of Vue.
We tried a handful of approaches to satisfy the test utils, but kept getting stuck in the same spot: appending the
<thead>
into the new<table>
simultaneously caused the<thead>
and<tbody>
to disappear from the original<table>
and nothing to render in the new header<table>
. Given that it worked in beta.29 and not in beta.30, I combed through the changelog and commits. Nothing jumped out, so I bisected commits between the two releases and finally identified the commit above.I'm not entirely up to speed with how the
_Ctor
reference works in Vue, but the issue was resolved as soon as I removed the_Ctor
stubs from thecomponent
reference. This revert did not cause any tests to fail, but I wish I could better reproduce the issue in testing to avoid having it re-introduced, as it's not an obvious issue.