[BUGFIX] Updates internals for Ember Canary

[pzuraq]

We just landed some pretty major changes to the Descriptor system
upstream. We verified that ember-concurrency would still be able to work
first, this branch is that work. We should likely update it to use
ember-compatibility-helpers as well, so you can continue to support
older versions of Ember at the same time.

[bendemboski]

Hey @pzuraq can you give me some advice/help on getting ember-concurrency-test-waiter to work with this update? Currently it kinda just monkey-patches task properties here, but that will no longer work.

So what I'm trying to figure out is a solution that will work with ember-concurrency both before and after this change, possibly taking this PR as an opportunity to provide a cleaner hook for what ember-concurrency-test-waiter is trying to do.

[pzuraq]

Ah, that's interesting. I think the only way for us to support this behavior would actually be to continue to add the taskFn to the decorator as a property, then have you replace it like you are currently 😕 we could also add it to a WeakMap potentially and make that map accessible, I'm not sure if that's much cleaner though

[bendemboski]

Well, it's kinda ugly and I'm not happy about how it encodes an assumption about the arguments that the framework passes to tp, but I found a way to make ember-concurrency-test-waiter work with this PR -- bendemboski/ember-concurrency-test-waiter@fc119c5.

@pzuraq I'm curious to hear your thoughts -- is there a cleaner way to effectively wrap a computed property/descriptor?

I suppose another approach would be to instrument the task property. Maybe:

• insert let taskFnWrappers = []; here
• insert tp.addTaskFnWrapper = function(fn) { taskFnWrappers.push(fn); } here that allows modifiers to register wrapper functions that are called with the task function (bound to its arguments) so they can run code before/after the task function.
• add some code here to wrap nest/wrap all the wrapper functions around the task function and pass that wrapped function to the task instance.

What are your thoughts on which approach would be best?

cc: @machty

[bendemboski]

Okay, I hashed out my second proposal, and I kinda like how it turned out. Here is the proposed change to this PR, and here is the corresponding change to ember-concurrency-test-waiter (and this pattern could theoretically be used by other addons that want to wrap ember-concurrency task functions if there ever are any).

I think it's nice and clean and doesn't violate encapsulation or encode brittle assumptions about how Ember (or even ember-concurrency) internals work. I imagine there are other abstractions that could accomplish the same thing, and I'm certainly open to them, but if folks are happy with this, maybe we can move forward with it?

@pzuraq I have some time to work on moving this whole thing forward if you can help me with:

1. Figuring out how to use ember-compatibility-helpers (what feature do we check for? does it exist yet, or does ember-compatibility-helpers need a PR too?)
2. Figuring out how to structure the code to be cross-compatible -- do I just take the whole diff in this commit and wrap every substantive change in an if block checking ember-compatibility-helpers (and then maybe organizing the code a little better, like putting the old code in a single file of helpers or something)? Or can you envision a cleaner way to organize the two mechanisms?
3. General support/help with questions, as there's a lot here I haven't learned about yet, so stuff might come up!

[pzuraq]

I'm ok with that proposal, I would like to see what @machty thinks since it would essentially be a new public API and may incur some runtime cost. The nice thing about using a WeakMap to get/set the function is it would be possible to strip that out in production builds, and keep it a private/intimate API.

For your remaining questions:

1. We've mostly been using the gte/lte flags from compatibility-helpers, they allow you to specify a version like so:

if (gte('3.9.0')) {

} else {

}

The first code block will run if the version is 3.9.0+, including betas and canaries. The additional flags were harder to maintain since there are so many of them, so we aren't really using them anymore.

2. You can take a look at ember-decorators to see how we handled it there, but generally yes. Sometimes it's possible to just tweak one or two things depending on different versions, but in this case we found it really required entirely different implementations.

3. Absolutely! Feel free to ping me on Discord about this, or ask more questions in this issue 😄 I'll also give you commit to my fork so you can iterate there if you'd like

[machty]

I will try to catch up on this over the weekend.

Could someone please summarize the main thing(s) that changed about Descriptors that are impacting ember-concurrency?

[pzuraq]

Descriptors are now spec compliant decorator functions, and have changed significantly under the hood. We have a new implementation that works, but it will be incompatible with older versions of Ember. This won’t be a problem as we can use ember-compatibility-helpers to ship the correct version.

However, a sideeffect of this is making most of the implementation details of descriptors much more private, and in turn making more of the implementation details of ember-concurrency more private. @bendemboski’s addon was relying on these private implementation details to create a test-waiter, so we need to figure out how to make them public-ish, or make it clear it’s not a supported use case.

[machty]

Still haven't dug in deeply but it seems like it's making more and more sense to have an official ember-concurrency middleware API for decorating/wrapping/extending task functions, so that separate e-c plugins don't have to know anything about ember-compatibility-helpers.

Random thought but I believe there's an issue with trying to wrap a task generator fn with yield * original; there's some cases where I think if your wrapped task fn returns a promise/task instance that throws an error, the error isn't properly handled by e-c. I noticed this ember-concurrency-retryable cc @maxfierke . This is another reason to have a sanctioned e-c wrapping/extending API.

[maxfierke]

yes, it subtly alters how the task runs. Basically, in vanilla e-c right now (no wrapping), the part before the first yield will run immediately, and errors will not be caught/handled by e-c. Whereas, wrapping will run it within the context of the e-c scheduler, and be handled in e-c (I think maybe it runs w/ the next microtasks?). I think this was essentially how it worked in ember-concurrency before 0.8.x, IIRC. (Examples are here: #216 (comment), for anyone interested)

It would be a subtly breaking change to wrap by default, but it's one that's already being incurred via either ember-concurrency-retryable or possibly via ember-concurrency-test-waiter. It seems like an Ben's approach to an API for taskFn wrapping would be a pragmatic way of solving this (until there's a more concrete idea how something like e-c middleware would work) provided that it did no wrapping if not invoked.

[pzuraq]

Status update here: Currently waiting on some pending changes to the implementation, it looks like we may revert it from the 3.9 beta and wait until 3.10 to land it due to that, give us some more time to handle things. Should know by the core team meeting on Friday!

[rwjblue]

FYI - we (but really @pzuraq) did revert the changes for 3.9 (current beta) over in emberjs/ember.js#17649, to buy us more time to experiment with the feature on canary. Thank you @pzuraq!

[bendemboski]

@pzuraq @rwjblue any idea when the reversion will land in the ember-beta channel that ember-try uses? I'm having to disable the ember-beta scenario in more and more addons that use ember-concurrency...

[pzuraq]

Just checked, it's looking like it'll be cut later tonight, but definitely by tomorrow at the latest. Sorry for the delays!

[pzuraq]

Beta.2 is out, should be fine!

[pzuraq]

This is ready to review!

[gossi]

I just tried your PR with the latest canary:

DEBUG: -------------------------------
DEBUG: Ember      : 3.10.0-canary+40ecde67
DEBUG: Ember Data : 3.8.0
DEBUG: -------------------------------

I had errors before, that went away -> #worksforme ;)

[buschtoens]

I can also confirm that machty/ember-concurrency-decorators#39 works with this branch! 🎉
https://travis-ci.org/machty/ember-concurrency-decorators/builds/502538049

[machty]

I still don't deeply understand the new vs old mechanics and limitations of descriptors and how that's impacted e-c (and I probably won't til I have more time and/or need to dig into to fix an low-level issue), but this PR looks good and I'm happy to merge.

@maxfierke does this PR still risk introducing the execution corner case bugs mentioned earlier?

@bendemboski does this PR now work with your e-c-test-waiters addon?

[bendemboski]

Yeah, thanks to this and related changes (and @pzuraq!) this branch works with ember-concurrency-test-waiters on canary and pre-canary https://travis-ci.org/bendemboski/ember-concurrency-test-waiter 🎉

[maxfierke]

@machty nope, should be okay, as taskFn is passed right on through with this PR. My comment about about the execution corner cases applied only to one of the proposals in here for a taskFn wrapping API, if taskFn were to no-longer be available on the TaskProperty directly.

It works with ember-concurrency-retryable too 😄