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: #455 #661

Closed
wants to merge 4 commits into from
Closed

fix: #455 #661

wants to merge 4 commits into from

Conversation

@38elements 38elements force-pushed the updated branch 3 times, most recently from be90415 to 372ab7b Compare May 27, 2018 04:50
@38elements 38elements changed the title investigate #455 fix: #455 and #661 May 27, 2018
@38elements 38elements changed the title fix: #455 and #661 fix: #455 and #582 May 27, 2018
@eddyerburgh
Copy link
Member

This is a good solution, but I'm not sure we should keep it long term.

The recent changes to run watchers synchronously has caused lots of bugs.

I have a PR open that will add an async option to Vue, so we can set that to make rendering work synchronously—vuejs/vue#8240

I think the solution when we have the async option is to remove the code that sets watchers to sync, because there are too many bugs. Instead we should warn that users with Vue < 2.6 must either update Vue or use Vue.nextTick for updates to the DOM.

One question with this PR—I don't think child components will call update with this solution?

@38elements
Copy link
Contributor Author

38elements commented May 29, 2018

Thank you for reply.
sync option is useful.
Since sync option is different from the original behavior of Vue,
I think that it is better to remove sync option.
Since if sync option is removed, I think it is not necessary to resolve #455,
I will remove code which resolve #455 and keep code which resolve #582.

One question with this PR—I don't think child components will call update with this solution?

I think update method of child component is replaced at below line.
I will add test.

https://github.com/vuejs/vue-test-utils/pull/661/files#diff-81317be9320c4ebacbb2460e0f504e58R28

wrapper.vm.foo = 'bar'
expect(spy.calledOnce).to.equal(true)
expect(fooSpy.calledOnce).to.equal(true)
expect(wrapper.html()).to.equal('<div>bar<div>bar</div></div>')
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eddyerburgh

One question with this PR—I don't think child components will call update with this solution?

Since this test passed, child components are updated.

@38elements
Copy link
Contributor Author

I will remove code which resolve #455 and keep code which resolve #582 within 3 days.

@38elements 38elements changed the title fix: #455 and #582 fix: #582 May 30, 2018
@eddyerburgh
Copy link
Member

I think this issue will be fixed in #586

@38elements
Copy link
Contributor Author

Since this change will be unnecessary, I close this pull request.

@38elements 38elements closed this May 31, 2018
@38elements 38elements deleted the updated branch May 31, 2018 13:49
@38elements
Copy link
Contributor Author

@eddyerburgh

I am sorry.
I change my opinion.
I think sync mounting option is necessary and should not be removed.
sync mounting option is useful.

It is not problem that sync mounting option is different from the original behavior of Vue.
It should be explicitly described in document that sync mounting option is different from the original behavior of Vue.

If the async option is added to Vue@2.6, it is not added to Vue < 2.6.
In this case, It is inconvenient to run the same test by different Vue versions.
At Vue@2.6+, it is possible to use async option is false.
At Vue < 2.6, it is impossible to synchronously test .
Or The expression in the test will become different depending on the Vue version.
I think it is better to abstract the difference between async option and existing sync mounting option by sync mounting option.

@eddyerburgh
Copy link
Member

Ok, can you reimplement your original code for update?

@38elements 38elements restored the updated branch June 1, 2018 13:04
@38elements 38elements changed the title fix: #582 fix: #455 Jun 1, 2018
@38elements 38elements mentioned this pull request Jun 1, 2018
@38elements
Copy link
Contributor Author

38elements commented Jun 1, 2018

@eddyerburgh
Thank you for reply.
I send pull request #675.

eddyerburgh pushed a commit that referenced this pull request Jun 1, 2018
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

2 participants