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

Overhaul store.push and store.pushPayload #161

Closed
wants to merge 7 commits into from
Closed

Overhaul store.push and store.pushPayload #161

wants to merge 7 commits into from

Conversation

pangratz
Copy link
Member

@pangratz pangratz commented Aug 1, 2016

@pangratz pangratz added the T-ember-data RFCs that impact the ember-data library label Aug 1, 2016
@pangratz pangratz changed the title ember-data: overhaul store push Overhaul store.push and store.pushPayload Aug 1, 2016
@@ -294,7 +294,11 @@ use cases are supported with this.
introduces those type of reference to the public
([`store.getReference`](http://emberjs.com/api/data/classes/DS.Store.html#method_getReference)
is not really helpful at the moment). It is questionable if returning
references is needed here. Since we have a `store.normalizePayload` now, it is
references is needed here and in general, if references are intented for this
Copy link

Choose a reason for hiding this comment

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

s/intented/intended/

@pangratz
Copy link
Member Author

pangratz commented Aug 7, 2016

I started working on a proof of concept https://github.com/pangratz/ember-data-store-push-improvements

@josemarluedke
Copy link

Our team would love to see this landed. We have our backend api as a thrift server and we use Ember Data in the frontend. Basically we had to create some random conversions from our api response to JSON API format. Having something like this would help us organize our code and most importantly, test it easily with patterns in place. ❤️

@sandstrom
Copy link
Contributor

sandstrom commented Aug 12, 2016

Nice writeup and thanks for tackling this! 🏆

Some thoughts:

  1. I'd prefer pushReference over pushRef. It'll be consistent with how BelongsToReference and HasManyReference are named. Also, this method call won't be repeated throughout the app (like myRecord.save()), so having a short method name isn't important.
  2. Is there a planned release date for Ember Data 3.0? If so we could keep this under a flag for a while and (possibly) go with store.normalize (instead of store.normalizePayload) as breaking change in 3.0.
  3. Instead of adding pushRef/pushReference, how about re-using store.push or store.pushPayload + an option, e.g. method(..., { reference: true }) to have it return references? To avoid having three push-like methods on store. Don't know if this is feasible though.

@pangratz
Copy link
Member Author

Hey @sandstrom, thanks for your feedback.

I've updated the RFC so there is a pushDocument now, which returns a DocumentReference.

I am not too convinced about the additional parameter, as this would return a different type for the method, depending on the passed in function. One could argue, that this is already the case since store.push either returns a record or an array of records. But I would still prefer a dedicated method for returning a reference.

@sandstrom
Copy link
Contributor

@pangratz sounds great!

I agree that an additional parameter (3) has some drawbacks. I was mainly trying to figure out was of keeping the API surface down. What do you think about using 3.0 to reduce the number of normalization methods on the store? But this isn't a major thing, overall I'm very much in favour of this proposal!

@pangratz
Copy link
Member Author

We should definitely make sure the normalization story is better in the future and tidy that up with 3.0 seems like a reasonable idea! Hopefully all this is fleshed out and implemented before we reach that 😃

@rwjblue
Copy link
Member

rwjblue commented Jun 19, 2019

I think we ultimately want to deprecate store.normalizePayload completely (its just not the responsibility of the store to do serialization/deserialization).

After todays ember-data core team meeting we moving this into "FCP to close".

@rwjblue rwjblue added the FCP to close The core-team that owns this RFC has moved that this RFC be closed. label Jun 19, 2019
@hjdivad hjdivad closed this Aug 21, 2019
@runspired
Copy link
Contributor

normalizePayload doesn't even exist anymore :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FCP to close The core-team that owns this RFC has moved that this RFC be closed. T-ember-data RFCs that impact the ember-data library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants