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

asap: add support for zone.js with microtasks #113

Closed
wants to merge 1 commit into from

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Apr 30, 2015

Would you please consider integrated this small patch into the master branch ?

Context:
This patch is required by Angular2.
In Angular2, we need to execute the change detection (check what properties have changed) at the end of each VM turn. A VM turn being one macrotask followed by 0+ microtasks.

By using this patch in combination with zone.js, we will be able to correctly detect VM turns - we need a custom version of zone.js, I'll submit the PR shortly.

Anything else I could do before this can get merged ?

Thanks.

/cc @mhevery

@jakearchibald
Copy link
Collaborator

Is the desire here to run promise jobs after other jobs, even if they're scheduled within the same job?

Eg:

setImmediate(_ => console.log(1));
Promise.resolve().then(_ => console.log(2));
setImmediate(_ => console.log(3));

The above will currently log 1,2,3, is the aim of this PR to make it 1,3,2 if zone.js is present? If so, I'm a little worried about it as it breaks es6.

@stefanpenner I guess this should go in RSVP rather than here anyway. What's your take on it? Should Angular fork here, or should there be a way to change the job queuing stuff?

@vicb
Copy link
Contributor Author

vicb commented Apr 30, 2015

The above will currently log 1,2,3, is the aim of this PR to make it 1,3,2 if zone.js is present? If so, I'm a little worried about it as it breaks es6.

This PR does not intend to change the order (ie it should be 1, 2, 3).

Nonetheless I need to update zone.js to enqueue a microtask in such a case, thanks for the pointer !

@stefanpenner
Copy link
Owner

@stefanpenner I guess this should go in RSVP rather than here anyway. What's your take on it? Should Angular fork here, or should there be a way to change the job queuing stuff?

RSVP actually makes the scheduler pluggable, I could expose something similar here.

https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/lib/ext/rsvp.js#L23-L32

@stefanpenner
Copy link
Owner

my further thoughts:

  • exposing the scheduler seems more reasonable and more extensible then including framework/framework integration
  • in auto-polyfil mode, this schedule change will affect all libraries not just libraries that expect these ordering changes. So we should be mindful of this, and ensure this doesn't cause un-expected issues.

note: in ember, we essentially do this, because the "whole world" abides by the run.loop. I believe this is the same in angular via zone.js and more or less reasonable.

cc @IgorMinar

@vicb
Copy link
Contributor Author

vicb commented Apr 30, 2015

@stefanpenner a more generic way to override asap() would be great !

@vicb
Copy link
Contributor Author

vicb commented Apr 30, 2015

@stefanpenner there would be no ordering changes. The purpose of this change is only to detect the end of VM turns (macrotask + microtasks).

@stefanpenner
Copy link
Owner

The way do this in ember, actually defers all of asap to the run-loop. I'll have to take some time to be sure my thoughts correctly align with zone.js Ultimately, I believe a more flexible scheduler would be of benefit to both es6-promise and rsvp.

I suspect something that is both compatible with ember run.loop and zone.js would allow for a reasonable scheduler API to be derived. I will also be sure to review bluebirds scheduler :)

@jakearchibald something like Promise._setSchedular(...) ?

@vicb you are correct, I forgot that zone.js is merely for detection/control. Can i assume, it correctly stub native promises?

@vicb
Copy link
Contributor Author

vicb commented Apr 30, 2015

@stefanpenner I'm not sure if there is a way to make zone.js work with native Promise. Our plan was to always use es6-promise (as a polyfill or a monkey patch).

The "issue" with native Promise is that you can't know a microtask has been en-queued before it is actually executed but we should know in advance because we don't want to execute the change detection after a macrotask when there are pending microtasks (unless I've missed something ??)

@stefanpenner
Copy link
Owner

(as a polyfill or a monkey patch).

ah ok.

@vicb
Copy link
Contributor Author

vicb commented May 4, 2015

@stefanpenner some more thinking about this (let me know if you prefer that I open an issue and copy the text there instead of discussing on this PR).

In Angular2:

  1. we need to polyfill (when native Promise is not implemented) or monkey patch (when implemented) because we should know when a microtask is scheduled, see my previous comment
  2. as of today, my current prototypes maintain a microtask queue which is drained after a macrotask has been executed, at the end of the run() method. This is where we need to override the asap() implementation.

I have realized that letting run() both schedule and execute microtasks is not ideal (angular/zone.js#97) because top level code could run outside of the zone.run(), ie:

<script>
// This code is not wrapped into zone.run()
Promise.resolve(null).then(function() {
  // will only be executed after the call to zone.run') :(
});
</script>

One way I can think of to solve this is to use JS for scheduling microtasks by using:

  • any of the method asap() currently support (nextTick, setImmediate, ...),
  • native Promise.then() when native Promise is available.

For this to work we would need:

  1. to be able to use the es6-promise implementation for asap() when native Promise is not available,
  2. to override asap() with a Promise based implementation when native Promise are available.

The zone.js implementation od scheduleMicrotask() would look like:

function scheduleMicrotasks(fn) {
  pendingMicrotasks++;
  asap(function() { // asap() being either the es6-promise polyfill or a native Promise based impl
    try {
      fn();
    } finally {
      pendingMicrotask--;
    }
}

item 2 was already discussed (ability to override asap())
item 1 is new to this discussion (ability to use the es6-promise impl)

This is my current thinking, I'll try to build a prototype and update here with my results.

@stefanpenner
Copy link
Owner

This sounds reasonable. I think making asap pluggable, and having zone.js provide the needed implementation is likely the best bet. Other things like RX and friends also have similar requirements. I suspect it will be the best out-come.

As, I mentioned RSVP.js has this feature. Although it uses more bytes to accomplish it, then I would like in this library (Typically byte sensitive people prefer es6-promise to RSVP)

@vicb
Copy link
Contributor Author

vicb commented May 5, 2015

@stefanpenner I have a prototype working - I can point you to the repos/branches if you are interested in the detail but the basic idea is:

// Override es6-promise asap implementation
export default function asap(callback, arg) {
  window.zone.scheduleMicrotask(function() {
    callback(arg);
  });
}

// zone.js scheduleMicrotask
var resolvedPromise = Promise.resolve(void 0);

function _asap(fn) {
  resolvedPromise.then(fn);
}

function scheduleMicrotask(fn) {
  // The idea is to use any of the es6-promise fallbacks when native Promise are not available
  _asap(this.bind(fn));
}

This implementation is not using the queue from the es6-promise implementation. I think there should be no reason why it would not work but may be it's better if you allow overriding this as well.

Let me know if I can help you port this from RSVP.

@vicb
Copy link
Contributor Author

vicb commented May 12, 2015

a better solution is implemented in #116

@vicb vicb closed this May 12, 2015
@vicb vicb deleted the zone2 branch May 12, 2015 12:06
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