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

Add promise support to nextTick #3967

Merged
merged 4 commits into from
Nov 20, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/core/instance/render.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export function initRender (vm: Component) {

export function renderMixin (Vue: Class<Component>) {
Vue.prototype.$nextTick = function (fn: Function) {
nextTick(fn, this)
return nextTick(fn, this)
}

Vue.prototype._render = function (): VNode {
Expand Down
18 changes: 11 additions & 7 deletions src/core/util/env.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,17 @@ export const nextTick = (function () {
}

return function queueNextTick (cb: Function, ctx?: Object) {
const func = ctx
? function () { cb.call(ctx) }
: cb
callbacks.push(func)
if (!pending) {
pending = true
timerFunc()
if (cb) {

Choose a reason for hiding this comment

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

cb is declared as Function, I would expect it as a non-nullable value. Maybe changing the cb annotation is better.

var func = ctx
? function () { cb.call(ctx) }

Choose a reason for hiding this comment

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

Well, if you changed cb to be nullable, then flow will report error here because it is pessimistic

: cb
callbacks.push(func)
if (!pending) {
pending = true
timerFunc()
}
} else if (typeof Promise !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, rationale for this instead of simply else if (Promise)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With a simple if, this would be thrown when Promise is undefined:

Uncaught ReferenceError: Promise is not defined

Copy link
Member

@fnlctrl fnlctrl Oct 18, 2016

Choose a reason for hiding this comment

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

I think if (window.Promise) {...} should do.
Sorry, suddenly realized vue also runs on node which doesn't have window

return Promise.resolve(ctx)
}
}
})()
Expand Down
17 changes: 17 additions & 0 deletions test/unit/features/instance/methods-lifecycle.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,5 +109,22 @@ describe('Instance methods lifecycle', () => {
done()
})
})

if (typeof Promise !== 'undefined') {
it('should be called after DOM update in correct context, when using Promise syntax', done => {
const vm = new Vue({
template: '<div>{{ msg }}</div>',
data: {
msg: 'foo'
}
}).$mount()
vm.msg = 'bar'
vm.$nextTick().then(function (ctx) {
expect(ctx).toBe(vm)
expect(vm.$el.textContent).toBe('bar')
done()
})
})
}
})
})
25 changes: 25 additions & 0 deletions test/unit/modules/util/next-tick.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { nextTick } from 'core/util/env'

describe('nextTick', () => {
it('accepts a callback', done => {
nextTick(done)

Choose a reason for hiding this comment

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

Why doesn't this return a promise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I didn't want to encourage people to mix the two styles (passing a callback vs chaining a promise).

Choose a reason for hiding this comment

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

The it sounds like separate another API is a better choice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand what you're saying, but are you suggesting having a separate nextTickPromise method or similar?

Choose a reason for hiding this comment

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

Yes, exactly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not force people to learn another method name. This convention of being able to use either a callback or a promise is also quite common, whereas I've never seen a library do as you propose.

})

it('returns undefined when passed a callback', () => {
expect(typeof nextTick(() => {})).toBe('undefined')
})

if (typeof Promise !== 'undefined') {
it('returns a Promise when provided no callback', done => {
nextTick().then(done)
})

it('returns a Promise with a context argument when provided a falsy callback and an object', done => {
const obj = {}
nextTick(undefined, obj).then(ctx => {

Choose a reason for hiding this comment

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

why ctx is passed as argument, I wonder. Because nextTick does not modify obj, the caller of nextTick can always access ctx. If we are using nextTick as a Promise fallback in which ctx is wrapped and consumer does not own handle to ctx, it is possibly useless because we can use native Promise. If promise does not exist, nextTick returns undefined which does add much value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for Vue.prototype.$nextTick, which binds the context to the callback or in the case of a promise, passes the context as an argument.

expect(ctx).toBe(obj)
done()
})
})
}
})