search

genkgo / ember-localforage-adapter

Offline usage for Ember Data, based on localstorage adapter, but now uses Mozilla's localforage as data source
Other
133 stars 26 forks source link

Discussion about keeping the loading process of relationships #42

Closed sebweaver closed 9 years ago

sebweaver commented 9 years ago

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:

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 commented 9 years ago

I created the branch issue-42 in which:

Please have a look to https://github.com/sebweaver/ember-localforage-adapter/commit/6daa38384dcf8baabd0cd6b831d0ea323705dc01 to see these important changes.

All test pass, and ELA is now thinner!

frederikbosch commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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

frederikbosch commented 9 years ago

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

sebweaver commented 9 years ago

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 commented 9 years ago

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

frederikbosch commented 9 years ago

Implemented.