Skip to content
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 two bugs in autodestroy #1723

Merged
merged 1 commit into from Oct 31, 2020
Merged

Fix two bugs in autodestroy #1723

merged 1 commit into from Oct 31, 2020

Conversation

pieterdd
Copy link
Contributor

@pieterdd pieterdd commented Oct 23, 2020

My colleague @Kjoep did some bugfixes. I'm filling out the form on his behalf to get it merged into the mainline.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • It's submitted to the dev branch.
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number) - It's not related to an existing issue thread as far as we know
  • All tests are passing: https://github.com/vuejs/vue-test-utils/blob/dev/.github/CONTRIBUTING.md#development-setup - Awaiting build
  • New/updated tests are included - The TypeScript change is not testable. Anything else the author should cover?

Other information:

@Kjoep
Copy link

Kjoep commented Oct 23, 2020 via email

@afontcu
Copy link
Member

afontcu commented Oct 23, 2020

Hi! Thanks for this 🙌 Could you add some unit test to assert that the expected behavior is actually happening?

@Kjoep
Copy link

Kjoep commented Oct 29, 2020

@afontcu Added a unit test - this one fails without the fix, but succeeds now.

Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, just need to revert the dist files - we only update those when we do a release.

@@ -11223,12 +11223,12 @@ function enableAutoDestroy(hook) {
isEnabled = true;

hook(function () {
wrapperInstances.forEach(function (wrapper) {
wrapperInstances.forEach((wrapper) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you revert this? we only update dist when we do a release

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, we've reverted the code change.

@pieterdd
Copy link
Contributor Author

We've also removed the second commit since it was obsolete after #1724 got merged:
image

@lmiller1990 lmiller1990 merged commit c198d19 into vuejs:dev Oct 31, 2020
@lmiller1990
Copy link
Member

I'll release in the next day or two, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants