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

EmberData | Request Service #860

Merged
merged 13 commits into from
Dec 2, 2022
Merged

Conversation

runspired
Copy link
Contributor

@runspired runspired commented Nov 11, 2022

Rendered

Proposes a simple abstraction over fetch to enable easy management of request/response flows. Provides associated utilities to assist in migrating to this new abstraction. Adds new APIs to the Store to make use of this abstraction.

@runspired runspired added the T-ember-data RFCs that impact the ember-data library label Nov 11, 2022
@runspired runspired changed the title [DRAFT] EmberData | Request Service EmberData | Request Service Nov 12, 2022
@runspired runspired marked this pull request as ready for review November 12, 2022 08:01
@runspired runspired self-assigned this Nov 12, 2022
@les2
Copy link
Contributor

les2 commented Nov 14, 2022

A better option IMO would be to:

  1. keep ember-data as is and see if there's a group that will maintain it if the core ember group no longer cares for it
  2. and create a new package for whatever is described in here that isn't even remotely ember-data

I don't think ember-data is perfect, but when it hasn't been a good fit i've had zero problems ember g service foo and using fetch directly within that service to interact with the API.

the general ember-data paradigm has worked well for me in practice building many applications over many years.

maybe some of us will have to fork this repo and put everything in a new namespace to keep our apps working without the churn of rewriting everything to fit in with some new model we didn't want.

$0.02 😭 😞


## Motivation

- `Serializer` lacks the context necessary to serialize/normalize data on a per-request basis
Copy link
Contributor

@les2 les2 Nov 14, 2022

Choose a reason for hiding this comment

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

I can't think of a time when I've needed to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amending requests and responses in the adapter happens quite often. In fact it turns out to be so necessary that both EmberData's default adapters do this and EmberData's core logic does this post-serialization but with-request-context in order to ensure that the cache receives enough context to correctly be patched. These special things EmberData is doing come at a high cost to flexibility and performance, and can't be rectified by user-land changes. So even if you aren't doing this, you are actually doing this.

I'd say every app that I've worked on has had to customize behavior in the adapter to support normalization or serialization at least once. It could be that I've hit some weird bias where in a hundred apps I haven't encountered the opposite, but it's also simply the case that more often than not APIs include meta in headers, return error responses in different formats, utilize binary formats to save bytes, or return "" or {} on success when something more was expected or needed.

Copy link
Contributor

@sandstrom sandstrom left a comment

Choose a reason for hiding this comment

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

Sounds awesome!

I took a quick look at our adapters and serializers. There are a few things that it would be good if this new paradigm supported (and maybe it does, and I've only missed it in the RFC).

  • Great if a handler can "pause a request" (not resolve/fulfill it), and initiate another request to the backend, fetching additional information, and only resolve the original request after the second request has completed.

    We use something like this today, to dynamically fetch translations via a special endpoint, in response to metadata stored on the regular response payloads.

  • Changing the HTTP type before a request hits the wire, for example changing GET to POST (we use this for certain search queries, if the query body is larger than the URL max limit). But reading the RFC, it sounds like this would be doable?

  • Our serializers currently do mostly one of these tasks: (a) skip certain attributes on the model from being sent over the wire and (b) transform model attributes, e.g. between string-date and moment-js-instance.

    It would be good if these common use-cases was easy to implement under the new paradigm. Maybe by storing metadata on the Model or ModelAttribute, and then reading these out in the handler and acting on them.

    Or, this logic could be moved closer to the model, e.g. by attaching serialization and de-serialization methods directly on models and model attributes somehow?

    This seems to me like a move towards lower-level primitives, which I don't have anything against! But if more logic will be customized outside Ember Data, it helps to think about easy tools we can add that make doing so easier. For example, being able to set metadata on model properties, that can somehow be read and acted on during serialization.

Overall I'm very positive! 🏅

Comment on lines +188 to +241
Handlers will be invoked in the order they are registered ("fifo", first-in
first-out), and may only be registered up until the first request is made. It
is recommended but not required to register all handlers at one time in order
to ensure explicitly visible handler ordering.
Copy link
Contributor

Choose a reason for hiding this comment

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

In some frameworks with a list of handlers invoked in a particular order (e.g. Ruby Rack), there are often methods such as:

  • insert
  • insert_before
  • move_after

They all help adding in additional "functions" at a specific index.

Could be useful here too. But something that can easily be saved for a later revision (not needed for v1). However, maybe insert would be a better name than use, because it's easier to add the sibling methods mentioned above (works better name-wise).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially a DAG was considered (similar to how initializers and addons can be ordered); however, the trouble with these approaches is while the ordering seems to make sense when viewing the late-add in isolation, the lack of explicit upfront ordering still generates high levels of confusion. If it turns out there's a strong use-case for just-in-time additions of handlers we can consider that in a new RFC.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense! At least for us, I don't see this as a problem.

Also, makes sense to keep the scope of initial RFC down a bit. As you mention, if this turns out to be a problem for a lot of people, one can always follow up later with additional API surface.

@runspired
Copy link
Contributor Author

  • Great if a handler can "pause a request" (not resolve/fulfill it), and initiate another request to the backend, fetching additional information, and only resolve the original request after the second request has completed.

Yes, making this trivial is one of the primary motivations of this pattern. There are two things enabled here:

  • first, you can do any amount of async within a handler
  • second, you may tee or split the request by calling next with a new query (and may do so any number of times). While the original query is immutable, you can copy parts of it forward to construct new queries and tee the request.

So for instance:

{
  async request(context, next) {
    const { records, url } = context.request;
    if (records && url.endWith('/users')) {
      const responses = await Promise.all(records.map(record => next(buildQuery(record))));
      // .. munge responses together
      return data;
    }
  }
}

The same goes for the inverse, you can join requests in a handler so long as the original requests resolve correctly. For instance to batch calls to findRecord you could defer each while pushing the requests into a map from which to later coalesce and resolve the requests

{
  requests = new Map();
  nextTick = null;
  
  async batchRequest(requests, next) {
     // call next as few or as many times as needed
  }
  
  async request(context, next) {
    if (context.request.op === 'findRecord') {
      this.requests.set(context, defer());
      this.nextTick = this.nextTick || setTimeout(() => {
        this.batchRequest(this.requests, next);
        this.requests = new Map();
        this.nextTick = null;
      }, 10);
      return defer.promise;
    }
  }
}

Changing the HTTP type before a request hits the wire, for example changing GET to POST (we use this for certain search queries, if the query body is larger than the URL max limit). But reading the RFC, it sounds like this would be doable?

Doable, and handler can continue the chain with a modified query. The power of this is that the cache will only ever be aware of the initial query. At first this sounds like a limitation, but it ensures that a broad set of abilities to fulfill data from sources of varying natures is neatly abstracted away. So from the cache's perspective, if what you initially queried was a GET, that is also what it will be checking against as part of it's cache-key. It's much easier to control the consistency of call-sites when constructing queries than it is to control the consistency of something that is mutated elsewhere.

Our serializers currently do mostly one of these tasks: (a) skip certain attributes on the model from being sent over the wire and (b) transform model attributes, e.g. between string-date and moment-js-instance.

It would be good if these common use-cases was easy to implement under the new paradigm. Maybe by storing metadata on the Model or ModelAttribute, and then reading these out in the handler and acting on them.

Serialization/Normalization is a handler concern for requests originating via the store. The reason we don't provide a direct replacement for serializers is because doing it inline like this brings enough power and simplicity that a few well crafted functions specific to your app will always beat any pattern we can design.

That said: (b) is mostly a separate RFC – coming soon – in which we will replace @ember-data/model. Transforms have always been somewhere between a design-flaw and an anti-pattern, and transforms that do things like between string-date and moment-js-instance are especially bad as they pollute the cache with unserialized state. Lossless Transformation between serializable primitive states becomes the concern of a utility used by a handler, hydration between serializable primitive state and a stateful class (such as a moment-js instance) becomes a concern of the presentation layer.

@sandstrom
Copy link
Contributor

sandstrom commented Nov 15, 2022

Yes, making this trivial is one of the primary motivations of this pattern.

Great, I assumed that would be possible (I think this architecture is likely a bit more powerful then what we currently need, but definitely a good approach!).

At first this sounds like a limitation, but it ensures that a broad set of abilities to fulfill data from sources of varying natures is neatly abstracted away.

Makes a lot of sense. In this particular example, that's exactly what you'd want (cache it as if it was a GET).

That said: (b) is mostly a separate RFC – coming soon

Sounds great!

Yeah, I think when this has landed, we'll probably also notice smaller 'pain points' when migrating from "the old way". And as we do, those pain points can be discussed and if a lot of others have similar issues, those can likely be remedied fairly easily.

Either via community "plugin handlers", by sharing best practices (e.g. in docs; overall I think docs are underutilized, in that everything doesn't need to be supported in a framework via public API, sometimes 'receipts' [this is how you do it] is better, and requires no additional API surface) or by making small additions to remedy.


Overall I'm very positive on these upcoming changes for Ember Data! 🏅


@runspired I've upped our sponsorship to $500. Do you know anyone else that will be working on this, where sponsorship may be appreciated?

@runspired
Copy link
Contributor Author

@les2 thanks for the feedback! Here are some thoughts on each bit.

A better option IMO would be to:

  1. keep ember-data as is and see if there's a group that will maintain it if the core ember group no longer cares for it

We very much care for EmberData, these changes allow us to evolve into a better future. The design proposed here allows us to support dozens of new use-cases, solve innumerable bugs, simplify one of the most complex aspects beginners and advanced users alike express regular frustration about, remove one of the biggest footguns when using EmberData (lack of pagination out of the box) and remove several entanglements with classic-ember (mixins, EmberObject, resolver usage).

  1. and create a new package for whatever is described in here that isn't even remotely ember-data

This is a new package, but it is also very much EmberData. What the RequestManager does is formalize the request process in a much more powerful way than has existed in the past, but it has always existed. I gave an overview of this at EmberFest 2019. Said differently: the "pipeline" for fulfilling a request has always been here, now we're giving folks the power to make use of it to solve far more problems.

I don't think ember-data is perfect, but when it hasn't[sic] been a good fit i've had zero problems ember g service foo and using fetch directly within that service to interact with the API.

What we are proposing here is that the same infrastructure that makes it super simple to fetch and work with modeled data would allow you to just as easily and simply fetch and work with raw data, with the added benefit that this coordination and standardization of how you fetch data results in both an improved developer experience when working in an app and an easier time maintaining and building applications as key request concerns can be coordinated centrally.

the general ember-data paradigm has worked well for me in practice building many applications over many years.

I'm glad <3

It has worked well enough for lots of folks, and I've seen it used to great power and effect and also to great misery and sorrow. I chose to invest in the project entirely because I believed it's paradigms could be improved such that the "worked well" becomes "worked amazing" as the rule, not the exception.

maybe some of us will have to fork this repo and put everything in a new namespace to keep our apps working without the churn of rewriting everything to fit in with some new model we didn't want.

I updated the RFC to describe a bit more about the transition path. I honestly doubt anyone will feel the need to fork, but if someone did want to maintain the legacy behaviors forever they could in-fact do so not by forking but by choosing to adopt and continue maintaining the legacy-handler, adapter, and serializer packages once we choose to end our own support. The power of the RequestManager approach is that these older legacy behaviors can still be described by it. The challenge would be around removing the Mixins, jQuery, RSVP, and EmberObject from the adapter and serializer package in order to maintain compatibility with Ember once those concepts are fully eliminated.

$0.02 😭 😞

Your $0.02 is hugely appreciated, and I'm glad to know another person who has gotten great utility out of this library in the past. Give this new pattern some trust and time. As with tracked vs computed it's not always instantly clear when paradigms massively change that the new world is just nicer and easier to breathe in: but I trust it will be having played with these patterns now for some time. But in order to ensure adequate time to build that confidence and embrace the new patterns the timeline set here maintains the old world for at least two major releases.

I'd go further and say we'll choose to maintain support for Adapter and Serializer for as long as Ember classic exists (likely until Ember 6.0) even if we ship our own 6.0 prior to that. It is overall low-cost for us to maintain this part of the codebase as long as Classic is around (the same is not true of Model, the changes to which will be a separate RFC), though specifically maintain means very occasional bug-fixes. These classes have been frozen in time due to their complexity for many years and that is unlikely to change.

@runspired
Copy link
Contributor Author

@sandstrom

Do you know anyone else that will be working on this, where sponsorship may be appreciated?

No, but if anyone out there would like to help there is (as always) much to do and it would be hugely appreciated <3

@runspired
Copy link
Contributor Author

Something this RFC doesn't explicitly layout but should: how to handle errors thrown by requests from the store generated due to a backgroundReload (see: emberjs/data#3809)

My thinking is:

Since the RFC gives control over catching and handling the error to the request-manager, end users can choose to swallow the error.

This allows us to consider two options:

  1. we could allow empty responses and ignore them, if a handler wished to swallow the error for a backgroundReload we would not generate a new error when seeing an empty response, we'd just move on.
  2. we could ensure that background requests that reject update the cache but are swallowed at that point (since we now have the full context that it was a background request throughout the system). This prevents consuming applications from being required to catch the error unless they wish to via a handler.

@sandstrom
Copy link
Contributor

Something this RFC doesn't explicitly layout but should: how to handle errors thrown by requests from the store generated due to a backgroundReload

Probably obvious, but I guess there are a few common reasons for errors during background reload:

  • user is no longer authenticated, session timeout etc (401)
  • user no longer has access to the resource (it still exists in the client-side store) (403)
  • the resource no longer exist (404)
  • server errors or temporary downtime (5xx)
  • network errors (no HTTP code, e.g. connection failures)

I'd say the response may vary depending on which one that triggered it.

  • 401: I think most apps will want to redirect to the login page.
  • 403: I guess one may want to keep existing client store [ignore], or clear it from client store
  • 404: Probably clear it from client store?
  • 5xx: likely show some error modal, or ignore
  • network error: likely ignore, keep stale/old state in store

@runspired runspired added Final Comment Period S-Exploring In the Exploring RFC Stage labels Nov 24, 2022
@runspired
Copy link
Contributor Author

Moving to FCP, barring further discussion will merge at earliest Friday December 2nd.

runspired added a commit to emberjs/data that referenced this pull request Nov 24, 2022
* feat: request-manager

* more docs

* more docs

* Add installation instructions

* fix link

* remove bad ref

* update docs

* more docs

* more niceness

* finish README doc

* full response docs

* stash work

* some updates

* cleanup compile

* delete file

* stash work

* update flow chart

* more updates to align with v2.1 cache

* update package name

* fix missing file

* more docs

* spike impl

* nice things

* test updates

* address feedback on readmes

* add request validator

* more tests

* more tests

* more tests

* polish streams support

* add abort controller support

* add Response handling

* fix lint

* more docs

* more docs

* rfc updates

* initial spike of new cache types

* update lockfile

* isolate changes

* isolate more things

* remove exp types

* udpate imports

* docs updates

* update docs

* finish overview

* fix lint

* fix test

* update lockfile

* skip fix for now

* fix tests

* fix unstable test

* we dont have prod tests for request manager yet so dont run them in prod

* actually fix url

* nvm

* fix content-length check
@runspired runspired merged commit 45aee05 into emberjs:master Dec 2, 2022
runspired added a commit that referenced this pull request Apr 5, 2023
Advance RFC #860 "EmberData | Request Service" to Stage Ready for Release
runspired added a commit that referenced this pull request Jul 28, 2023
Advance RFC #860 `"EmberData | Request Service"` to Stage Released
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period S-Exploring In the Exploring RFC Stage T-ember-data RFCs that impact the ember-data library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants