Polymer / polymer

Our original Web Component library.
https://polymer-library.polymer-project.org/
BSD 3-Clause "New" or "Revised" License
22.04k stars 2.01k forks source link

Split `Polymer.Base.async` into macrotask and microtask callbacks #1305

Closed cdata closed 2 years ago

cdata commented 9 years ago

Microtask asynchrony seems to be fundamentally different from what you get when you use setTimeout and friends. This has led me to unexpected (but significant) timing differences between using async with 0 (or no time) and 1.

Also, when reading code like this:

this.async(function() {
  // stuff..
}, 1);

It is not obvious why the 1 is specified.

For anecdotal evidence of ergonomic frustrations with Polymer.Base.async, please refer to https://github.com/Polymer/paper-ripple/pull/32#discussion_r26625650 .

arthurevans commented 9 years ago

I agree that this difference in behavior is not clear. The difference between adding something to the microtask queue and the event queue is significant.

I wonder whether it would be better to have an explicit API for microtask async. (For example, Dart has a separate scheduleMicrotask() API.)

pflannery commented 9 years ago

@arthurevans I agree, maybe queueMicroTask(cb) and queueMacroTask(cb, timeout) would be better.

pflannery commented 9 years ago

whilst on the subject, would a Promise.resolve().then(atEndOfMicrotask).catch(...some error handling...) be a better choice for creatnig microtasks than using a mutation observer with a text node? here

sorvell commented 9 years ago

The async defers a function close to the minimum amount of time possible or a specified amount. It's unclear that we need another specific signal to indicate another interval of time. Can you explain the time frame you'd like to elapse before the function is called?

cdata commented 9 years ago

The crux of the issue is that this.async(...) and this.async(..., 1) do significantly different things. Without understanding the implementation of this.async, it could lead unwary users into hard-to-diagnose timing issues. A potential resolution is to have separate methods for microtask and timer asynchrony. E.g., this.async for timer, and this.microtask for microtask.

cdata commented 9 years ago

Here is a direct link to the original style discussion I had several months ago that prompted the issue: https://github.com/Polymer/core-focusable/pull/12#discussion_r26275349

dfreedm commented 8 years ago

Closing due to age.

cdata commented 8 years ago

I removed pending-response because I gave a response. Almost a year later, I am continuing to wait patiently for a meaningful response to this issue.

devinivy commented 8 years ago

@cdata are you looking for docs to be improved, for the API itself to be made more clear, or for an explanation of why the current API is as it is?

I will say, I very much like that the timing of async() without arguments matches the timing that is used to propagate data-binding and the timing to move from attached to the equivalent of "dom-ready". This is very useful.

Would async() supporting the full gamut of running asynchronous tasks be better? Perhaps async(cb, 0) could provide macrotask timing.

E.g. async(cb) --> microtask timing async(cb, x) --> behaves like setTimeout()/rAF(), macrotask timing

cdata commented 8 years ago

are you looking for docs to be improved, for the API itself to be made more clear, or for an explanation of why the current API is as it is?

I'm seeking an improved API. My proposal above is to split async into two methods: one that queues microtasks, and another that queues "macrotasks."

I will say, I very much like that the timing of async() without arguments matches the timing that is used to propagate data-binding

I agree that this seems useful.

Would async() supporting the full gamut of running asynchronous tasks be better? Perhaps async(cb, 0) could provide macrotask timing.

In fact, your example is how async already behaves today, and I'm arguing that this is a poor design that has led to confusion (and will continue to do so).

davidgiven commented 8 years ago

I have pretty much never found async useful as it stands --- every time I need to use something like it, it's because I need to wait for the browser to do some calculation which only happens at the end of an event loop such as layout, or event delivery, or some such thing. Microtasks aren't useful for this.

I would definitely support splitting it into two functions, or at least blessing the async(cb, 0) syntax so I can use it.

(Otherwise I'll have to roll my own.)

devinivy commented 8 years ago

@cdata it appears as though the async() utility treats 0 as microtask timing. The utility defers to the code here https://github.com/Polymer/polymer/blob/master/src/lib/async.html#L20-L28. I think that it should behave like setTimeout() when 0 is passed.

cdata commented 8 years ago

Thank you, I am familiar with the code in question. "Micro" task and "macro" task asynchrony are fundamentally different enough with regards to timing that they deserve separate, distinct methods. The current API leads to confusing outcomes. Many users of the current API have never heard of microtasks, and therefore lack the basic understanding needed to apply them appropriately. A user cannot truly understand the implications of the second argument (or lack thereof) without reading and understanding the implementation of the method itself.

On Wed, Mar 2, 2016, 7:09 AM devin ivy notifications@github.com wrote:

@cdata https://github.com/cdata it appears as though the async() utility treats 0 as microtask timing. The utility defers to the code here https://github.com/Polymer/polymer/blob/master/src/lib/async.html#L20-L28. I think that it should behave like setTimeout() when 0 is passed.

— Reply to this email directly or view it on GitHub https://github.com/Polymer/polymer/issues/1305#issuecomment-190881453.

devinivy commented 8 years ago

Ah, sorry I was confused because I misunderstood you to mean that async(cb, 0) already behaves like setTimeout(cb, 0). It's a lot easier to say "when there is a second argument async() behaves identically to setTimeout() in terms of timing, otherwise async() uses microtask timing." Currently we cannot say that because of the behavior of async(cb, 0). I see now from your original post that you are familiar with this specific bizarrity of async().

In any case, I appreciate your point that these ideas are not familiar to many folks, and that having separate methods makes sense. Perhaps Polymer could also be a force in educating developers about the sort of timing difference. It eventually becomes important to understand this difference in Polymer-land, given that some crucial events occur with microtask timing.

arthurevans commented 8 years ago

Tagging @sorvell and @kevinpschaaf here. I, too, found this confusing.

At the minimum, as @cdata says, we should try to educate users about the difference. @kwalrath wrote a great article for Dart several years ago:

https://www.dartlang.org/articles/event-loop/

I would love to adapt this, but all of the syntax is Dart-specific, so it would take a fair amount of work. But the diagrams and explanations are very clear and helpful.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 2 years ago

This issue has been automatically closed after being marked stale. If you're still facing this problem with the above solution, please comment and we'll reopen!