Discussion about keeping the loading process of relationships

[sebweaver]

Currently, when ELA fetch records it also takes the responsibility of loading all their relationships.

This raises several issues:

1. The process doesn't take care of the async characteristic of the relationship (true or false). In other words, in applications where relationships are set to be async (which is now the default in Ember Data 2.0), ELA will do a lot of unnecessary work for nothing. All the more as this work can be very expensive when a lot of records with a lot a nested relationships are fetched.
2. The process applies on non embedded associations whereas this is specially the kind of relationships for which the async mechanism is intended to (which brings us back to point _#_1).
3. The process is not symmetrical among all the find and query methods: findAll() should also call loadRelationshipsForMany(), as findMany() does.
4. The process fails when a relationship can't be satisfied (i.e. element's id doesn't exist).

Considering all of these points, my question is: do we still need to maintain this complicated and unnecessary behavior?

My opinion is: no, we don't. I think the adapter should limit its responsibility by focusing only on the basic CRUD operations, those ones required by the store, and let the latter handles its own sophisticated mechanisms. ELA would be more lightweight and consequently more maintainable. We could stick to ED changes more easily too.

The counterpart of this decision is it would introduce some possible breaking changes on existing applications using ELA:

• set { async: false } to embedded relationships and let the default to the non embedded ones
• handle the Promise returned by async relationships (rather than a direct read of the value)
• adapt the inherited adapters and serializers (if any and if necessary)

However, this could not be that important, because ED 2.0 introduced breaking changes too. As a matter of fact, ELA migration could be part of the overall migration process to ED 2.0.

Happy to read your POV about all of this.

Regards,

Sébastien

[sebweaver]

I created the branch issue-42 in which:

• I did some cleanings in the tests to improve their readability before migration (8 commits, from 9775d21 to 6351c7a)
• I removed all the loading process from the Adapter
• I adapted the Models in order to:
  • Remove async property from all non embedded relationships (default to { async: true })
  • Leave { async: false } as is on embedded relationships (i.e.Customer model)
• I adapted the tests to handle the promise of async relationships
• I removed the normalization process from the Serializer (since the embedded payload doesn't exist anymore).

Please have a look to sebweaver@6daa383 to see these important changes.

All test pass, and ELA is now thinner!

[frederikbosch]

@sebweaver I think I am in favour of all of this. There is a lot of code coming from the localstorage adapter. This project is originally forked from that project. With these changes, we could now get rid of unnecessary code, synchronous characteristics from localstorage and start benefiting more from asynchronous characteristics of localforage.

But, keep in mind, since we still save everything in one localforage key, the whole database will still be read into memory when fetching records the first time. And caching mechanism reduces the number of calls to localforage itself. I think this PR reduces load on DS.Store, but will not be much more memory efficient.

So, for the record, could you indicate which models need an upgrade after this merges in? I mean for people that have async: true things will still work, right? And for people with embedded records, if I understand things correctly, there are no breaking changes either.

[frederikbosch]

Depending on your reply, we could decide when introduce the breaking changes. Your PRs to Ember Data will probably be merged into ED 2.1.0 so maybe we could introduce these 'breaking' changes together with removing the current workarounds around those PRs. And then we release ELA 2.1.0.

[sebweaver]

Performance

About that, I totally agree with you. These changes improve the load on the store, but due to the current implementation (i.e. one unique key to contain the whole database) it doesn't change anything on the memory footprint. The latter is not the purpose of this PR (and I have other ideas about that very topic...)

The purposes were:

1. Make the code more maintainable for future fixes, improvements or features
2. Respect the way end users design theirs models and relationships (async or not, embedded or not, specialized serializers or not) by leaving these mechanisms to ED
3. With point _#_2, restore the SRP of an Adapter (supposed, at least)
4. Make the project more flexible to stick to ED releases, by minimizing layer entanglements
5. Eventually, improve the CPU load is just a side-effect of the previous objectives ;-)

Needed changes

You're right. The migration process is quite simple and almost natural if done in the same time of migration to ED 2.0 (which introduced { async: true } by default anyway). To get an idea, have a look at what I've done in models and tests of ELA to make them work altogether again. All relationships are now async (not set by default) except those requiring explicit embedded records.

Release plan

Your plan for releasing these changes in 2.1 sound good to me, specially about removing the workarounds in the same time.

I thought about some new test cases which seem to be missing in the current test suite, but I don't known when I can do that. So, do you want the PR as is, right now, and I'll add these new tests in another dedicated PR? Or do you prefer I'll add them in the same PR but later?

[frederikbosch]

@sebweaver Alright, let's move this in with ED 2.1. I think the PR does not require any changes. I only want to ship an upgrade guide with the release since we might break things.

Is it correct that only people that will be affected by this change are people using a relation by model.get('relation').get('relationProperty') instead of model.get('relation').then and are not using the embedded records mixin?

[sebweaver]

Yes, indeed. To be clearer, here is a table which should cover the different migration scenarios:

If my relationship was...	And my associated records were...	My relationship now must be...	What about my association getters?
{ async: false } or not specified	embedded	{ async: false } explicitely	No change
{ async: false } or not specified	not embedded	{ async: true } or not specified	Use promise instead of a direct read (see example below)
{ async: true }	not embedded	No change	no change
{ async: true }	embedded	irrelevant	irrelevant

Migration example for affected getters:

var record = model.get('association')
record.get('whatever');
record.doSomething();

Becomes:

model.get('association').then(function(record) {
  record.get('whatever');
  record.doSomething();
})

Please feel free to correct/improve/enrich this migration helper.

[frederikbosch]

@sebweaver Wonderful. I extracted the table to the upgrade guide. Do you agree?

[frederikbosch]

Then you could create a PR of your commit and we merge it in.

[sebweaver]

On second thought, the above table is actually squashing the changes required by ED migration and the ones required by ELA, whereas some of them are introduced at different version.

So I think it's even clearer to split the table, as follows:

Changes required by migration from ED < 2.0 to ED >= 2.0:

If the relationship was...	The relationship now must be...
{async: false}	no change
{async: true}	no change or specification could be removed
not specified	{async: false} explicitely

Then the following changes are required by migration from ELA 2.0 to ELA 2.1:

If the relationship was...	And the associated records were...	The relationship now must be...	What about the association getters?
{async: false}	embedded	no change	no change
{async: false}	not embedded	{async: true} or not specified	Use promise instead of direct read (see example below)
{async: true} or not specified	not embedded	no change	no change
{async: true} or not specified	embedded	irrelevant	irrelevant

Plus the same example as before.

[frederikbosch]

@sebweaver Alright, I will change that. Can you create the PR?

[frederikbosch]

Implemented.