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(setData): allow empty objects to be set fix #1704 #1705

Merged
merged 3 commits into from
Oct 7, 2020

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Oct 1, 2020

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:

if someone relied on empty objects not updating, that now no longer makes sense

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

This is a tentative fix to show one possible solution, I can clean it up if you think it makes sense

fixes vuejs#1704 

This is a tentative fix to show one possible solution, I can clean it up if you think it makes sense
@lmiller1990
Copy link
Member

Seems like some tests are failing. LMK if I can help out with anything.

@Haroenv
Copy link
Contributor Author

Haroenv commented Oct 5, 2020

I didn't update the tests yet, but I'm more wondering if my approach makes sense, and whether this is a problem that should even be fixed @lmiller1990 :)

@lmiller1990
Copy link
Member

I guess so, if Vue supports this (which it does, right, settings something in data to {} will not lose reactivity), it seems like VTU probably should too.

@Haroenv Haroenv marked this pull request as ready for review October 6, 2020 07:26
@Haroenv
Copy link
Contributor Author

Haroenv commented Oct 6, 2020

Tests are passing now, although I didn't add a new test for this yet :)

@lmiller1990
Copy link
Member

Great. Can you add a test for this, too? Then we can merge it.

@Haroenv
Copy link
Contributor Author

Haroenv commented Oct 7, 2020

voilà @lmiller1990, new test added :)

Copy link
Member

@afontcu afontcu left a comment

Choose a reason for hiding this comment

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

🎉

@lmiller1990 lmiller1990 merged commit 993b293 into vuejs:dev Oct 7, 2020
@Haroenv Haroenv deleted the patch-1 branch October 7, 2020 14:15
@Haroenv
Copy link
Contributor Author

Haroenv commented Oct 7, 2020

Thanks for your reactivity all 👍

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

3 participants