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 store.push to accept { data: null } #3866

Merged
merged 1 commit into from
Oct 29, 2015
Merged

Conversation

bmac
Copy link
Member

@bmac bmac commented Oct 16, 2015

Closes #3790

cc @wecc could review this pr?

if (data.data === null) {
return null;
}

var internalModel = this._pushInternalModel(data.data || data);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this special case (data.data || data, I've personally never really liked it) should be considered if we return early if data.data === null above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this works because the the return null; never gets hit if data.data is undefined. I'd like to remove this || data from this line if possible. It seems like we could do that if we pull one of the assertions out of _pushInternalModel and put it into this method.

@igorT
Copy link
Member

igorT commented Oct 26, 2015

@wecc are we good here?

@wecc
Copy link
Contributor

wecc commented Oct 26, 2015

LGTM!

@bmac squash?

@bmac
Copy link
Member Author

bmac commented Oct 28, 2015

@wecc squashed.

igorT added a commit that referenced this pull request Oct 29, 2015
Allow store.push to accept { data: null }
@igorT igorT merged commit 31bf071 into emberjs:master Oct 29, 2015
@igorT
Copy link
Member

igorT commented Oct 29, 2015

Maybe a warning for people upgrading that we switched to json api?

@jherdman
Copy link

Would it be possible to back-port this to 2.2.0?

@bmac
Copy link
Member Author

bmac commented Nov 17, 2015

@jherdman yes I can release a 2.2.1 tomorrow with this fix.

@bmac bmac deleted the issue-3790 branch November 25, 2015 22:23
@bmac
Copy link
Member Author

bmac commented Nov 25, 2015

Sorry for the delay @jherdman. I just published 2.2.1 with this commit.

@jherdman
Copy link

No worries! I greatly appreciate the release.

CvX added a commit to CvX/ember-local-storage that referenced this pull request Feb 24, 2019
The mentioned issue has been fixed in ember-data 2.2.1 on Nov 25, 2015 (emberjs/data#3866 (comment)). The oldest version ember-local-storage currently supports is 2.12.
CvX added a commit to CvX/ember-local-storage that referenced this pull request Feb 24, 2019
The mentioned issue has been fixed in ember-data 2.2.1 on Nov 25, 2015 (emberjs/data#3866 (comment)). The oldest version ember-local-storage currently supports is 2.12.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants