EmberData | deprecate legacy finder support

[runspired]

Rendered

[wagenet]

We agreed at the RFC Review meeting today to move this to exploring. There's still a little bit more work to done before we accept.

[btecu]

It seems that this would reduce DX quite a lot?

Loading a record currently:

return this.store.findRecord('user', 1);

Loading a record after this RFC:

import { findRecord } from '@ember-data/json-api/request';

const result = await this.store.request(findRecord('user', '1'));
const user = result.content.data;

return user;

Maybe I'm missing some of the benefits, but I'm concerned this change would push many users away from ember-data, which judging only by the number of NPM downloads might already be at a low point (fewer downloads than other addons, e.g., ember-truth-helpers).

[runspired]

Loading a record currently

I think this example is flawed both because you aren't awaiting the promise, e.g. the correct code snippet is:

const user = await this.store.findRecord('user', '1');
return user;

And because (1) 50% of Ember apps dropped EmberData because of the inflexibility that the above example has and (2) a lot of the remaining users (finger in the wind I'd guess ~50%) don't use findRecord in this way because of the flaws it has. For those apps the minimum replacement is this:

const user = await this.store.findRecord('user', '1', { reload: true });
return user;

The change here allows you to trust the cache in a way these apps never could. Especially once includes or other query params comes into play.

Next, consider what's going on here:

const result = await this.store.request({
  url: '/api/v1/users/',
  headers: new Headers([['Accept', 'application/vnd.api+json']])
});

(The builder import is just an abstraction so you don't have to think about building urls and fetch options unless you actually need to).

This is far more succinct and ergonomic than what most apps have historically done to work around all the problems with adapters:

const user = await this.store.findRecord('user', '1', {
  reload: true,
  adapterOptions: { use: 'json:api', namespace: 'api/v1' }
});

^ and then a series of very hacky things in adapters to try to make those adapterOptions turn into something for the url and headers, which I won't try to replicate here as it takes dozens of lines of code.

Which incidentally brings me to the point that this older mechanism requires you to maintain a complex mental model and adopt thousands of lines of confusing adapter/serializer logic in your codebase. The couple of lines above in the call to request literally replaces many KBs of minified compressed logic you've been working around.

Finally, you have to consider what this line is actually giving you:

const user = result.content.data;

let's break this down:

• result gives you full access to the request and response objects. Meaning that where before accessing meta or other info from response headers wasn't possible now it is.
• content gives you full access to the response document. Meaning that where before accessing meta and links or errors information either wasn't possible or required many hurdles to jump through to work around, now its just there.
• data is the record, just now with this extra information tagging along.

So we see, the request point dx is largely improved. For the response the only real difference here is that you have this expressive and highly useful access to all of the information you might want. You trade const user = result for const user = result.content.data. I doubt anyone is going to abandon EmberData for needing to do this, and we'll very likely expand our user base massively given that ~50% of all ember apps use EmberData mostly because the other 50% struggled too hard with findRecord (and similar) for the very things this fixes.

fewer downloads than other addons, e.g., ember-truth-helpers

ember-truth-helpers is a pretty special case, most addons are not installed by 100% of ember apps like it is.

which judging only by the number of NPM downloads might already be at a low point

ember-source is ~135k/week and ember-data is ~95k/week and both have been very stable at those download levels for the last year. I actually suspect we're trending up to, a lot of apps seem to have made the jump to 5.x or adopted 5.x despite the massive set of deprecations in 4.x

A final thought:

I'm reminded of the transition to strict templates and the (similar to yours) sentiments that no one would want to make the transition: e.g. why {{@model.myThing}} instead of just {{myThing}}. The reason is the same and I don't think anyone today would go back.

[btecu]

Thanks for the explanation!

And because (1) 50% of Ember apps dropped EmberData because of the inflexibility that the above example has and (2) a lot of the remaining users (finger in the wind I'd guess ~50%) don't use findRecord in this way because of the flaws it has. For those apps the minimum replacement is this:

If it makes any difference, I've worked in about a dozen of Ember applications using both REST and JSON API adapters and I've never ran into that issue or had to add { reload: true } to findRecord.

This is far more succinct and ergonomic than what most apps have historically done to work around all the problems with adapters:

const user = await this.store.findRecord('user', '1', {
  reload: true,
  adapterOptions: { use: 'json:api', namespace: 'api/v1' }
});

I did use adapterOptions in the past for some non-standard endpoints that had a special URL and having that at the adapter level made a lot of sense. Similarly, if a resource is available under a different namespace, it would make sense to just set that once in the resource's adapter.

Finally, you have to consider what this line is actually giving you:

const user = result.content.data;
let's break this down:

result gives you full access to the request and response objects. Meaning that where before accessing meta or other info from response headers wasn't possible now it is.
content gives you full access to the response document. Meaning that where before accessing meta and links or errors information either wasn't possible or required many hurdles to jump through to work around, now its just there.
data is the record, just now with this extra information tagging along.

It's good the raw response is now accessible but I'm not sure how used it will be.
Right now, in most cases this is how I use findRecord:

model(params) {
  return this.store.findRecord(someResource, params.id);
}

For failed requests, including > 500, the error route is entered and a static page is displayed.
Validation errors and such are returned in the errors object and displayed to the user.
I'm not sure if this pattern is universal but I wouldn't be surprised if it was popular.
So with the proposal, we'd just have to do:

import { findRecord } from '@ember-data/json-api/request';

model(params) {
  let result = await this.store.request(findRecord(someResource, params.id));

  return result.content.data;
}

As you can see, it's more cumbersome as there's more ceremony for a simple request.
If someone doesn't need access to result or content and just wants data, will always need this extra step to return data. Perhaps there is another option to opt-in or opt-out, even if it's at the application level?

You mentioned 50% of users struggled with ember-data findRecord because they didn't had access to result or content. That seems very high, but still, maybe we shouldn't swing the other direction and force resource.content on the other 50% if possible.

I should add that GET requests are the least affected, at least in terms of number of lines and additional ceremony.
Create and updates are much worse. Update went from:

model.name = 'Chris';
await model.save();

to

import { recordIdentifierFor } from '@ember-data/store';
import { updateRecord, serializePatch } from '@ember-data/json-api/request';

user.name = 'Chris';

const request = updateRecord(user);
request.body = JSON.stringify(
  serializePatch(
    store.cache,
    recordIdentifierFor(user)
  )
);

await store.request(request);

Finally, this is just a nit - I dislike this function call as an argument. I'm not sure if this was done to keep the number of lines closer to current implementation or to have the example shorter:

const result = await store.request(findRecord('user', '1'));

vs

let whateverThisIs = findRecord('user', '1');
let result = await store.request(whateverThisIs);

Thank you for working to improve ember-data!

[runspired]

For failed requests, including > 500, the error route is entered and a static page is displayed.
Validation errors and such are returned in the errors object and displayed to the user.
I'm not sure if this pattern is universal but I wouldn't be surprised if it was popular.
So with the proposal, we'd just have to do:

There's kinda a few points to make on this.

1. you still just return the request. If it errors or hangs you still enter the loading or error states. That error state is massively more useful now too, because you actually have a lot more context on the error and you don't have all the weirdness of most the error info disappearing.

2. the use of record.errors is definitely an anti-pattern at this point. The replacement for @ember-data/model won't have the concept. This is a longer convo for another RFC but the presence of the errors proxy on the model isn't a reason to want to eliminate the outer wrappers. The reality is that if you want errors then you do want the content property.

3. You can just do this. In fact you often should just do this.

import { findRecord } from '@ember-data/json-api/request';

model(params) {
  return this.store.request(findRecord('user', params.id));
}

The utility of this becomes even more clear once you look at how applications actually use EmberData. Very few applications in my experience rely on the simple form of findRecord. Most rely on query, findAll, or findRecord+includes. In the case of all three of these things, if you were using them then historically you may as well have not been using EmberData. Let's dig into why:

For findRecord + includes, you have no guarantee of a network request. If the record is already in the cache, even if in a partially loaded state, then you will not hit network, leading to application errors. If you used coalescing or you triggered the same findRecord from multiple places you would similarly encounter application errors due to missing data. This is why both turning off coalescing and adding reload: true to all requests became so prevalent.

For findAll you encounter a similar problem. If even a single record for that type is in the cache already, even if its a locally created one, you won't hit network at all. So again, folks have dealt with this by adding reload: true to every request.

For query you were guaranteed to hit network every time. In fact, you had no other option. query could never resolve from cache, and so folks would either write a lot of code to add a very limited form of caching (which request above does far better), or they would result to using fetch and pushing the result into the store.

Further , with query pagination, meta and links usage was very difficult. With request.content these things are trivial and pagination is built in.

maybe we shouldn't swing the other direction and force resource.content on the other 50% if possible.

On the contrary, you are pointing out exactly why we do want to do this. You've already mentioned two of the primary reasons to do this. (1) the need otherwise to customize adapters using brittle heuristics that are assumed to be universal and rarely are. And doing so using very hacky work arounds to attempt to thread enough context to them to achieve what was needed. And (2) access to errors. If you want access to errors then you want the response content, not the record.

On Create/Update

I've set this into a different section because it honestly deserves its own analysis. Is the below verbose? Yes. Its also though the most verbose it will ever be.

import { recordIdentifierFor } from '@ember-data/store';
import { updateRecord, serializePatch } from '@ember-data/json-api/request';

user.name = 'Chris';

const request = updateRecord(user);
request.body = JSON.stringify(
  serializePatch(
    store.cache,
    recordIdentifierFor(user)
  )
);

await store.request(request);

Importantly, this does 100% of the work that adapters, serializers and custom-fetch did before. In other words, we've replaced a thousand of lines of complex logic with a couple of simple assignments. Which is nice, it makes the mental model far easier. Especially because I've yet to encounter an app for which record.save() was the happy or common path.

Nearly every app I've worked on has multiple services or model methods dedicated to working around the limitations of save. Whether its transactions (saving multiple records), partial saves, patching, cascading delete, saving relationships, or just trying to deal with the fact that a lot of endpoints turn out to be non-standard and need a little special handling in the url or of the response.

Especially because now you can save only what you wanted to save, you don't have to do crazy hacks in order to support saving a single attribute or just a couple of attributes, you don't even need a record to do a save, and you can even control the timing of ui updates, deciding whether to be optimistic or pessimistic. Even better, less need for crazy rollbackAttributes work arounds for failed saves or when leaving a component or route.

But can we do better? Sure. We intentionally gave the maximally verbose example because that is the solution that solves the problems that have caused most people to bail.

Here's the minimal one:

import { updateRecord } from '@ember-data/json-api/request';

user.name = 'Chris';
await store.request(updateRecord(user));

As opposed to (the non @ember-data/model way) of

user.name = 'Chris';
await store.saveRecord(user);

The only real difference is that there is an import.

As far as the "nit" goes, well, that's whats lovely about composition patterns. Sugar and style them however you'd like!

If for your app you'd like to hide things just a bit more, then either use a service to build the fetch options or use builders that accept the store as an argument. For instance:

import { saveRecord, findRecord } from 'my-app/builders';

const user = await findRecord(store, 'user', '1');

user.name = 'Chris';
await saveRecord(store, user);

Or if you want, the service approach:

export default class Store extends StoreService {
  findRecord(identifierOrType, idOrOptions, maybeOptions) {
    const result = await this.request(findRecord(idenfitiferOrType, idOrOptions, maybeOptions));
    return result.content;
  }
}

[jenweber]

The learning team reviewed this at today's meeting and have a couple questions.

• What are the known blockers to moving this to "accepted?"
• Does the Request Service itself need to be moved to "recommended" before this reaches a certain stage?

This will help us figure out priorities and work order.

Advancement PR: #942

We also reviewed the to-do list for making Request Service. I will break it into issues, and will help ask for contributors.

[btecu]

It would be great if the RFC had concrete examples of how things are currently done or things that can't be done today and how it would done if the RFC is implemented.

Just saying adapters are complex or confusing doesn't seem sufficient.

[runspired]

@jenweber RE

• What are the known blockers to moving this to "accepted?"
• Does the Request Service itself need to be moved to "recommended" before this reaches a certain stage?

For the first, the blocker is rewriting the guides and tutorial.
For the second, the blocker is mostly also rewriting the guides and tutorial, but we also need to replace async relationships. It moving to recommended is not a pre-req for this RFC as we intentionally do not change async relationship pattern recommendations in this RFC. We will change async relationship patterns in a future (but coming very very soon) RFC.

[runspired]

@btecu that was the request RFC. Many years ago at this point.

But also

Just saying adapters are complex or confusing doesn't seem sufficient.

I think saying what I have is sufficient. Replacing a thousand lines of code that no one understands or can debug with a few lines of code that everyone can understand and debug doesn't seem to need to say much more than that.

[ef4]

Status update here is: ember-data and learning are working to improve the documentation story for the features that replace this feature before we want to move forward with actively deprecating it.

[runspired]

checklist for advancing this

• update super rentals tutorial (PR Update the Super Rentals Tutorial to use the Request Service ember-learn/super-rentals-tutorial#238 )
• update emberjs website guides
  • https://guides.emberjs.com/release/getting-started/anatomy-of-an-ember-app/
  • https://guides.emberjs.com/release/models/finding-records/
  • https://guides.emberjs.com/release/models/creating-updating-and-deleting-records/
  • https://guides.emberjs.com/release/models/pushing-records-into-the-store/
  • https://guides.emberjs.com/release/models/handling-metadata/
  • https://guides.emberjs.com/release/models/customizing-adapters/
  • https://guides.emberjs.com/release/models/customizing-serializers/
• add request api docs
• add request guides
• provide temporary legacy builders for syntax-only shift
• provide codemod for syntax-only shift
• audit api docs for legacy syntax examples and migrate them to builders

[ef4]

This is expected to move forward after the newer async-relationship support lands.

[runspired]

We haven't landed all the pre-flight work and relationship work is still in-progress but its not a requirement for a design that the final implementation details are done before it lands, so moving to push this ahead provided we add details around the new "linksMode" we have been working to add to relationships to ensure they can be used with the requestManager without adapters and serializers.

We should also call out that we will need new docs for linksMode and a new guides section around "how to work with relationship/pagination links when your API does not provide them for you"

[kategengler]

@runspired Should this be merged or have the FCP label removed?