ampersand-state
ampersand-state copied to clipboard
AmpersandJS
Allow passing { parse: true } to set
Code looks fine, tests are valid, so +1 from me, as it's only added value and no potential risk. Thanks @jackboberg!
Gah, apparently I cannot comment on lines that you didn't edit. At a glance I'm reasonably sure we'll need to remove the parse in line 14, otherwise on new model initialization with { parse: true } we're gonna call parse twice.
Removed the constructor call. Would you like me to add a specific test to ensure parse is only called once?
I think as long as the other tests pass that's fine IMO
Philip Roberts &yet
On 26 Feb 2015, at 12:39, Jack Boberg [email protected] wrote:
Removed the constructor call. Would you like me to add a specific test to ensure parse is only called once?
— Reply to this email directly or view it on GitHub.
I had cherry-picked this change into our app, but just discovered an issue with it. I'm storing Amp.States in a Backbone.Collection, and if you call collection.set(data, {parse : true}), the parse function will actually get called twice: once by BB.Collection as it's looping over the incoming data and finds an existing model instance, and then the second time when it actually calls State.set() and passes in the same options.
Overall, the current pattern is that whoever owns the State instance is responsible for doing:
var parsedData = stateInstance.parse(data);
stateInstance.set(parsedData);
BB.Collection does that already, it looks like Amp.Collection does that as well, and in both cases they then pass in the same options object down to State.set(), in which case this code change causes it to try to re-parse the already parsed data.
Ultimately, I think this change is useful in the case where you're calling State.set() directly, but I don't know how to get it to keep from double-parsing in the case where it's being run from Collection.set().
