officially support mutating middleware about compose HOT 19 CLOSED

@jonathanong The problem occurs if you call app.use to add any middleware after you've already triggered a call of app.callback, since this.middleware, the argument that is passed to compose is a mutable array.

this would previously work fine as it would reference the now mutated array, but since the flatten changes the semantics by copying at call time of compose with the flatten, anything added to the middleware after the call to compose is effectively ignored.

I would think that even if this wasn't an envisioned use case, it still seems like a breaking change since it does, even if inadvertently, change the way the module works.

Thanks for all the hard work!

from compose.

Previously, one could mutate the array after calling compose.

Interesting, I had no idea that was possible. I'll rewrite flatten to be lazy.

from compose.

Interesting, I had no idea that was possible.

Yeah, admittedly our usage does feel a tad unusual..

I'll rewrite flatten to be lazy.

Thanks! I suppose it'd be a nice micro optimization (remove a copy) if you do, regardless of 'dynamic' usage like this..

from compose.

Thanks! I suppose it'd be a nice micro optimization (remove a copy) if you do, regardless of 'dynamic' usage like this..

Actually, this is really bad for performance, as it means it has to be flattened every time (previously it could be cached). In fact, that makes me reconsider this, maybe we should just remove variadic argument support... making the next PR anyway.

from compose.

Okay, I've opened #68 and #69 to solve this. However, I'm personally in favor of just reverting the variadic argument change, as the performance cost is now high.

from compose.

agreed, I would vote to revert the changes published in 2.5

from compose.

👍 +1，I'm having the same issue. It should be a break change.

from compose.

@jonathanong Might want to revert those changes sooner rather than later.

from compose.

can you guys explain your use-cases? this wasn't something we were ever supporting.

how much performance decrease are we talking about by doing the dynamic version?

from compose.

I'm not entirely sure how koa uses koa-compose but this change definitely breaks my application. Can you revert it?

from compose.

Having same issue, it is breaking our App.

from compose.

It seems that a patch along the following lines could preserve the object ownership, support variadic args, and not introduce any copying or dependencies:

function compose(middleware) {
  if (arguments.length > 1) {
    middleware = Array.prototype.slice.call(arguments);
  }
  // ...
}

This wouldn't support nested flattening, e.g. compose( [a,b], [c] ). Not sure if that's considered an important feature.

from compose.

I think compose([a, b], [c]) is pretty useful if you're working with lists of middleware. Maybe we should run benchmarks on these different implementations or something.

from compose.

yeah i'm gonna revert it now.

The problem occurs if you call app.use to add any middleware after you've already triggered a call of app.callback

@conorhastings why are you doing this, though?

from compose.

also, if this breaks usage within a 3rd party middleware/router/etc., which ones?

from compose.

i have published v2.5.1 and v3.2.1 which reverts the change. v2.5.0 and v3.2.0 are marked as deprecated on npm. let me know if you have any issues with these releases.

• changing this topic to "officially support mutating middleware". i see this as code-smell and would like to understand the use-case
• moving discussion to continuing variadic support in #63. this is still a feature i would like to add, but we will do so only in koa-compose@4

from compose.

@jonathanong it was likely a mistake in our app code, not sure why we were doing it, definitely not purposefully relying on array mutation 😕 . Have already refactored our app code so that it will work with 2.5 , haven't experienced anything caused by external modules myself.

In my opinion I think variadic support is much more important than supporting apps that rely on array mutation of an argument already passed into a function, would just like to see it published as a major version

from compose.

@conorhastings that's good to know!

yeah, you should be sure to do app.listen() or app.callback() only after you've added all your middleware. i think this is an oversight in the docs.

from compose.

instead of supporting it here, we will add an ability to configure your compose function in koa koajs/koa#1568

from compose.