Shallow-clone of options object could cause problems

[georgecrawford]

In cdf3d09dbc157b8979df607cb9ed4ebe3c42d14d I've introduced a (failing, pending) test, which demonstrates how a shallow clone of the options object on Module instantiation might not be enough.

I noticed exactly this case in my app:

• the options were defined in a JS object
• a module was instantiated
• a class was later added, and the module was rendered
• the module was destroyed
• a new module, using the same original JS object for config, inherited the additional class

This can so easily cause bugs which are hard to pin down, as it'll depend on a previous sequence of events. In my example, if an alert overlay was given a 'loading' class, all future alert overlays would also get that class.

Options to resolve this include:

• I ensure the JS object is initialised fresh for every instantiation, or is deep-cloned prior to being used each time
• FruitMachine does a deep-clone rather than a shallow clone in the constructor (see https://github.com/ftlabs/fruitmachine/blob/master/lib/module/index.js#L45)
• Each of the non-scalar components of the options object (classes, helpers, template, model, data, children) are shallow-cloned during Module#_configure. This won't help much though, as child elements will still be references.

What do people think?

[matthew-andrews]

Interesting. What would you expect to happen here:-

var model = new Model();
model.set('name', 'Matt');

var orange = new Orange({
  model: model
});

model.set('name', 'George');

console.log(orange.model.get('name')); // ???

I think I would that to log out 'George'.

Of the instantiation API I would probably expect the following to be deep-cloned:-

• children*
• helpers
• classes

And I would expect the same object to be preserved for:-

• model

* actually I would expect the first level to be cloned, deeper down should be up to the constructors of the children themselves to deal with decided what gets deep cloned or not.

[georgecrawford]

Yes, that makes a lot of sense. Model should definitely be preserved as-is, so the two-way binding can be retained. Helpers and Classes should be fresh deep clones, but I'm not so sure about children - you're more experienced to comment here. Your suggestion sounds good.

[sekoyo]

I ran into the same problem where classes[] was defined and then new instances kept appending to the same array unknowingly.

Not sure about deep cloning other stuff like children, I don't know enough about fruit but sounds like it could be memory inefficient? But might make sense for classes since you might expect it to work per instance in a reliable way (have the classes you originally set, not what another instance did to it).

Otherwise people just have aware to create instance local stuff in initialize instead.