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

Support embedded relations (embed: always-style) #24

Merged

Conversation

perlun
Copy link
Contributor

@perlun perlun commented Apr 16, 2015

Hi,

As promised in #22, here is a PR that adds support for embedded: 'always' style data.

I have also added the accompanying tests, and an example of how to set up the serializer for these cases.

Questions/suggestions? Let me know. All tests are green of course, and I will start using this in our own app now (since everything seems to work correctly).

Without this PR, the adapters causes errors since it tries to load data for relations where the child data is already available, which is obviously a problem.

@perlun
Copy link
Contributor Author

perlun commented Apr 16, 2015

(No Travis set up for the repo? 😛)

@perlun
Copy link
Contributor Author

perlun commented Apr 16, 2015

Ah, now I see that the Travis stuff is coming up. Nice!

@frederikbosch
Copy link
Contributor

@perlun Thank you for the hard work. I will do my best to investigate this as soon as possible. Could you explain beforehand what there is to gain with embedded: 'always'?

@perlun
Copy link
Contributor Author

perlun commented Apr 16, 2015

Could you explain beforehand what there is to gain with embedded: 'always'?

Sure. The point is when building the app, retrieving data perhaps via the DS.RESTAdapter or similar, sometimes it makes a lot of sense to "retrieve all data at once". Suppose for example you have an order. You might want to retrieve (and return) not just the order, but also the order lines, and maybe other data as well.

Of course, you can retrieve it all using asynchronous relations (the default pattern w/ Ember Data), but from our experience (building quite a lot of enterprise apps using Ember), the "embed always" style of data is sometimes a lot easier to work with, making implementation of the app less cumbersome. You have all data accessible at once, and there is no need for the .then() spaghetti which may otherwise occur... 😄

@frederikbosch
Copy link
Contributor

Hmm. You did read about caching in order to reduce the number of requests to one? Then all data is in memory, making your app a lot faster.

Regarding the then() spaghetti, I rather have a clear API using then() than not knowing for sure what I get. But I will invetigate the PR and challenge again.

@perlun
Copy link
Contributor Author

perlun commented Apr 16, 2015

@frederikbosch - just to make things clear. Our use case is not limited to using the localforage-adapter. It is a mix of an offline and an online store, with some requests going to the online store, others going to offline (and some going first to offline with online afterwards. :) )

So we can't really optimize for "localforage only" scenarios but need to think very carefully on how to build a data model that works well for both REST and localforage scenarios. In our experience, embedded: 'always' is often a superior approach to doing complex object graphs when you don't really have full control over how and when data gets loaded. It also makes it easy when you want to sync back the data back to the server (because you can, in simpler cases, just propagate the full object graph up to the REST adapter).

In any way, the EmbeddedRecordsMixin is an integral part of Ember Data and we (and others, like http://miguelcamba.com/blog/2015/03/15/optimizing-apis-with-ember-data-and-embeddedrecordsmixin/) find it very useful. You certainly don't have to use it if you don't like (and there might very well be cases when it doesn't make sense), but I think it is nice if the localforage-adapter supports it.

Simply put: In our app(s), it makes sense to design the data model like this.

frederikbosch added a commit that referenced this pull request May 6, 2015
Support embedded relations (embed: always-style)
@frederikbosch frederikbosch merged commit 37a5e80 into genkgo:master May 6, 2015
@frederikbosch
Copy link
Contributor

@perlun Thanks for your hard work, much appreciated! I have merged your code and tagged a new version.

@perlun
Copy link
Contributor Author

perlun commented May 7, 2015

Cool @frederikbosch, thanks for that!

@perlun perlun deleted the feature/support-embedded-relations branch May 7, 2015 09:11
@srsgores
Copy link

Hey @perlun, would it be possible for you to provide an example of how you were able to combine both the REST adapter and the localforage adapter for your objects? Do you just set the adapter for the top-level model to RESTAdapter, and the lower-level models to LFAdapter? I'm dealing with a similar case where it makes sense to take the same approach, storing values as-you-go, and pushing the final result to a server.

@frederikbosch
Copy link
Contributor

@srsgores You could have a look at Ember Sync. Because Ember Sync was in alpha stage when I started this adapter, I use the LFAdapter together with a RemoteManager. The latter acts as repository and contains methods like fetchUserData. Such a method pushes data into the store (which is injected into the manager).

@frederikbosch
Copy link
Contributor

@srsgores I also just noticed Ember Fryctoria. It was created by a contributor of this adapter.

@perlun
Copy link
Contributor Author

perlun commented May 13, 2015

@srsgores - apart from the suggestions by Frederik, here's how I did it:

  • Create a custom store that uses LFAdapter.
  • Set upp serializers that extend LFSerializer for the relations. Like this (CoffeeScript):
define('uxf/serializers/offline/order-draft-line', () ->
  App.LocalForageSerializer.extend(
    DS.EmbeddedRecordsMixin,
    attrs:
      deliveryDays: embedded: 'always'
  )
)

(we don't use ember-cli so hence the module definition here is a bit different to what you would expect.)

I then use a serializerFor override to set this serializer to be used for the offline store. (I use an offline store and an online store, with the calling code being explicit about which one to use.)

@srsgores
Copy link

@perlun, ingenious. Thanks for describing your process. I will update this with any hurdles I come across in my own implementation. I think we should consider adding some of these "hybrid" adapter approaches to the readme, at least until ember 2.0 comes out. What do you guys think?

@srsgores
Copy link

@perlun, where does the serializeFor live? Inside the main application serializer or in the adapter somewhere? Any code examples would be appreciated.

@perlun
Copy link
Contributor Author

perlun commented May 13, 2015

You create a custom store using something like:

Application.OfflineStore = DS.Store.extend
  serializerFor: (type) ->
    typeName = if typeof(type) == 'string' then type else type.typeKey
    # locate the proper serializer here and return an instance of it

It could also be that you can make it work by just giving the serializer the right name, and Ember will find it automagically. But the problem then is that it will be used for both the offfline and online stores.

(if you need more help, I can send you our hourly rates.... 😉)

@frederikbosch
Copy link
Contributor

@srsgores @perlun Great idea to add a list of approaches to the offline/online scenario, because that is why this adapter was created in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants