broken mixin helper for deep copy

[JeanSebTr]

Hi,

the mixin helper is broken for deep copy (it seems to alway have been) but, is it really necessary to use deep copy there ? Here's a quick fix : https://github.com/JeanSebTr/barista/blob/v0.0.8-fix/lib/helpers.js#L21

I expect it won't be broken for next versions since they will be using coffe-script's mixin :)

Github doesn't allow to make a pull request on a tag. So, if you consider this enough to release a new version before the big rewrite, I would make a pull request against a 0.0.8 based branch.

[kieran]

Huh, looks like a bug we haven't hit since it never deep mixes in practice :-/ Did you run into the error while using it?

Does the current implementation work? https://github.com/kieran/barista/blob/master/lib/route.coffee#L429

could always replace with a shallow mixer:

mixin = ( ret, mixin )->
  ret[name] = method for name, method of mixin
  ret

or for multiple args

mixin = ( ret, mixins... )->
  for mixin in mixins
    ret[name] = method for name, method of mixin
  ret

The big rewrite is almost done, honestly. I'll take a stab at finishing it tomorrow or Thursday.

[JeanSebTr]

Oh I didn't check the new code, I thought coffee-script had built-in sugar like this.

The current implementation works indeed. The bug in <= 0.0.8 was simply an undefined mixin reference (VS exports.mixin).

I feel it's not necessary to prevent mutation of original objects. e.g.: express doesn't do deep copy when merging response.locals with app.locals

In fact, I even got a "RangeError: Maximum call stack size exceeded" when passing directly data from mongoosejs. So, in my opinion it would be better with a shallow mixer.

I'm dynamically generating the routes from the database to customize urls for every client's web site. Urls are routed to generic controllers and I'm using the second parameter of .to() to pass specific behaviors defined in an object. It's how I catched the bug.

Don't feel in a hurry to release the next version, git dependencies in npm work fine for deployment :)

[kieran]

Cool - guess it's "fixed" then :-)

We could probably prevent circular references from exceeding the call stack by checking hasOwnProperty (via own):

mixin = ( ret, mixins... )->
  for mixin in mixins
    ret[name] = method for own name, method of mixin
  ret

Though it's unlikely we'll run into this issue since we're basically just mixing objects that we create / control, but if it doesn't hurt, why not!