emberjs / data

WarpDrive is a lightweight data library for web apps — universal, typed, reactive, and ready to scale.
https://api.emberjs.com/ember-data/release
MIT License
3.04k stars 1.33k forks source link

pre-rfc: deprecate store.pushPayload and store.serializeRecord #8815

Closed runspired closed 11 months ago

runspired commented 6 years ago

Similar to https://github.com/emberjs/data/issues/8873,

The RequestManager paradigm is simply that any request starts by providing FetchOptions. These may be built programmatically (for instance for relationships or collections that return links information from the API), or by builders or even manually.

Users have the choice of providing the body for a request at the point of the request, or inserting one later using a request handler. For both cases, EmberData provides access to the cache and its APIs for diffing state changes, as well as serializePatch and serializeResources utils for each cache implementation that we ship.

The legacy pattern's inflexibility meant that users often needed to eject from using adapter/serializer paradigms to fetch data. The new paradigm does not have these constraints, so we wish to deprecate the following methods that served only as work around and only work with these legacy primitives:

What to do instead

For Modern Apps

For Apps still using Legacy

RuslanZavacky commented 6 years ago

That probably requires a little bit more of explanation, here is a use-case that I have:

I use pushPayload a lot, as I do interact with API through simple requests, get JSON API in return, and push to ember-data when I know that I will work with those records. As I see it, ember-data should be aware of how to hydrate supported format (json-api) into the models. It just fills weird, that for sort-of standard operations, something custom is required. Serializers are part of the ember-data, as well as JSON API format itself.

Can we get a little bit of context, why this 4 lines for pushPayload can't stay in ember-data?

runspired commented 6 years ago

@RuslanZavacky

As I see it, ember-data should be aware of how to hydrate supported format (json-api) into the models.

This is exactly how store.push works and is what it is for.

Serializers are part of the ember-data

If you are already in json-api format, there's no reason for a serializer. Additionally, serializers may not remain in ember-data much longer in their current form. We're working on a more robust primitive to take their place.

why this 4 lines for pushPayload can't stay in ember-data?

It isn't 4 lines. It's much more once you consider the corresponding serializer code and a terrible amount more still once you consider that app engineers must align their payloads to a hidden, highly inefficient, non-json-api format for the method to even work. Furthermore, an unfortunate design mistake means that for most applications, the corresponding serializer hook has zero context on the payload being normalized (it does not receive the modelName included in the call to store.pushPayload).

mike-north commented 6 years ago

It isn't 4 lines. It's much more once you consider the corresponding serializer code and a terrible amount more still once you consider that app engineers must align their payloads to a hidden, highly inefficient, non-json-api format for the method to even work. Furthermore, an unfortunate design mistake means that for most applications, the corresponding serializer hook has zero context on the payload being normalized (it does not receive the modelName included in the call to store.pushPayload).

Can this be added to the RFC? What's this unfortunate design mistake? Why is it important for the serializer hook to have context around the payload being normalized?

sdhull commented 6 years ago

Considering my own confusion around these two methods, as well as watching the comments around this stuff recently, I would propose updating docs to make it clearer that store.push() is for payloads already in json-api format, and if your json is not in json-api format, then store.pushPayload() + custom serializer is (current-day) how you should push the json into the store, but going forward you will need to convert your json payload into something that is json-api compliant & use store.push().

Currently, the docs section titled Store createRecord() vs. push() vs. pushPayload() reads:

push is used to notify Ember Data's store of new or updated records that exist in the backend. This will return a record in the loaded.saved state. The primary use-case for store#push is to notify Ember Data about record updates (full or partial) that happen outside of the normal adapter methods (for example SSE or Web Sockets).

pushPayload is a convenience wrapper for store#push that will deserialize payloads if the Serializer implements a pushPayload method.

Nowhere does either paragraph mention json-api vs non-json-api. I think that simple distinction would have helped me a lot.

sdhull commented 6 years ago

@runspired I've been trying to convert our pushPayload calls to use push and have encountered an issue.

It seems that store.push() expects type: ${modelName} to be singularized, even though model.serialize() produces a type that is pluralized (and it appears that our API pluralizes model names by default).

When you have an application that uses the JSONAPIAdapter, shouldn't store.push(model.serialize({includeId: true})) not produce an error?

I guess that json-api spec is "agnostic about inflection rules"... which implies both singularized & pluralized type strings should work with store.push. Right?

sdhull commented 6 years ago

I was playing around on ember-twiddle and for the life of me I can't figure out why the serialized representation is different in testPush vs testPushPayload

runspired commented 6 years ago

@sdhull feel free to ping me on slack or discord to avoid sidetracking the conversation here; however, the short answer to your troubles is that serialize is that the store expects singular, dasherized types and camelCase members; however, as you noted the spec is agnostic to whether type is pluralized or not and dasherized or not. There's an assumption (largely a hold-over from the rails active-record days) that APIs largely work with pluralized types. Something we seek to do soon in ember-data is make it agnostic: what you give us is what you get. You must be consistent (and we will enforce consistency), but there won't be any other restrictions.

One of the downsides of the existing serializer implementation is that folks have never come to realize the criteria for what the store actually needs, resulting in folks aligning to the serializer spec instead of the store spec. Folks using json-api shouldn't require a normalization method at all if aligned, but unfortunately they haven't known what to align to.

That said, even for the simple case of fixing types, authoring your own serializer is often cleaner and many orders of magnitude more maintainable and performant. I left a comment with a demo of this here: https://github.com/emberjs/data/pull/4110#issuecomment-417391930

NullVoxPopuli commented 6 years ago

ping me on slack

discord? :trollface: https://discordapp.com/invite/zT3asNS

wagenet commented 2 years ago

I'm closing this due to inactivity. This doesn't mean that the idea present here is invalid, but that, unfortunately, nobody has taken the effort to spearhead it and bring it to completion. Please feel free to advocate for it if you believe that this is still worth pursuing. Thanks!

runspired commented 2 years ago

we're still planning on tackling this

runspired commented 1 year ago

flushed out the description of the RFC to be tackled

runspired commented 11 months ago

handled by https://github.com/emberjs/rfcs/pull/964