Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow passing { parse: true } to set #109

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jackboberg
Copy link
Contributor

Closes #106 #84

@jackboberg jackboberg changed the title Allow passing parse: true to set Allow passing { parse: true } to set Nov 21, 2014
@bear bear added the request label Nov 21, 2014
@kamilogorek
Copy link
Contributor

Code looks fine, tests are valid, so +1 from me, as it's only added value and no potential risk. Thanks @jackboberg!

@ngryman
Copy link

ngryman commented Feb 25, 2015

Hey guys, are you planning to merge this in a near future?
Thanks!

@@ -1542,74 +1542,31 @@ test('#68, #110 mixin props should not be deleted', function (t) {
t.end();
});

test('#114 setOnce allows values to be set once and only once', function (t) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackboberg It looks like a few tests are being removed here. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, that's a rebase gone awry...I'll get it fixed up tonight

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-rebased, thanks for the note.

@latentflip
Copy link
Contributor

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.

passing `{ parse: true }` will trigger during `set`
@jackboberg
Copy link
Contributor Author

Removed the constructor call. Would you like me to add a specific test to ensure parse is only called once?

@latentflip
Copy link
Contributor

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.

@markerikson
Copy link

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().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

{ parse: true } not passed to children
7 participants