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

Allow for polymorphic async relations #1491

Closed
wants to merge 1 commit into from
Closed

Conversation

ssured
Copy link
Contributor

@ssured ssured commented Nov 6, 2013

Async relations are wrapped in a DS.PromiseObject, if so, unwrap before querying the typeKey

Async relations are wrapped in a DS.PromiseObject, if so, unwrap before querying the typeKey
@hjdivad
Copy link
Member

hjdivad commented Nov 6, 2013

cc @cyril-sf

@bradleypriest
Copy link
Member

This will definitely need a test of some kind

@ssured
Copy link
Contributor Author

ssured commented Nov 6, 2013

This pull makes usage like below possible. I do not have a Ember Data setup available on my machine to create the test myself. I patched the serialization in my own Serializer.

Item = DS.Model.extend
  name: DS.attr 'string'
  linkedTo: DS.belongsTo 'item', {async:true, polymorphic:true}
NumberedItem = Item.extend
  number: DS.attr 'number'

item1 = store.createRecord 'item', {name:"item1"}
item1.save().then ->
  item2 = store.createRecord 'numbered_item', {name:"item2", number: 2, linkedTo:item1}
  serializer = store.serializerFor item2.constructor
  serialized = serializer.serialize item2
  expect(serialized.linkedToType).to.equal 'numbered_item'

@stefanpenner
Copy link
Member

i still think we need to drop non-async relationships and make them all async..

@CodeOfficer
Copy link
Contributor

I REALLY want to see all relationships made async ... this has been a source of great pain for me.

@ssured
Copy link
Contributor Author

ssured commented Nov 7, 2013

Is there any documentation on async relationships and promises? This issue kind of surprised me, I did not know before about PromiseArray and PromiseObject classes. What are the pros/cons of the current situation and pros/cons of moving all code into async?

Another thing I wonder is the debugability of promises. The nesting is quite hard to understand and very labor intensive. Alternative promise implementations exist, does it make sense to use these? Some claim to have a nice stacktrace (bluebird) and be very fast. I guess when moving all data related code into async promises its good to review performance.

@stefanpenner
Copy link
Member

@ssured although bluebird is quite fast unless you are actually spawning thousands or tens of thousands of promises, a simple ajax request or DOM mutation is so much more costly, that the promise implementation makes basically 0 difference. The real win for bluebirds performance is when using it server side, and you actually have thousands or tens of thousands (or more) open connections, all utilizing many many promises.

long stack traces of debatable usefulness. To be honest for me having the ability to label a promise, and give the exceptions native stack trace is far simpler and yields much more useful results. I've found in my longtrace spike of RSVP, the traces basically just told me that the error happened to flow threw several promises before I finally got it, which seems totally useless, the only piece of value was the final stack.

ideally improve the native develop tools can help mitigate some of these issues. For example, @teddyzeenny and myself are working hard to add a promise section to the ember inspector emberjs/ember-inspector#76 Which allows you to visualize the promise topology and as errors and values propogate.

Now before I said performance difference doesn't really matter, with the instrumentation hooks needed to make ^^ work, I would feel more comfortable with some of those performance improvements, as ideally leaving the instrumentation hooks in for production would yield the best debugging. My early suite of benchmark.js benchmarks show, the improvements to RSVP put us with bluebird in most situations. (sometimes faster sometimes, slightly slower)

Although lots of work to go, once we have ^ in place we should have some pretty slick promise debugging. Additionally we have begun working with vendors, to help make choices when it comes to how they themselves will improve debug ergonomics of their own DOM Promise implementations.

@ssured
Copy link
Contributor Author

ssured commented Nov 7, 2013

@stefanpenner Thanks for your elaborate answer. The idea of extending the ember inspector with Promises support is amazing!

Back to the async -> making all relations async, what's the pro/con?

@stefanpenner
Copy link
Member

@ssured no problem, I have given the async problem a metric-fuckton of thought, and have spent much time absorbing other peoples wisdom on the matter.

the pro:

  • async is the superset of functionality.
  • no matter how you change how data is loaded, your code will continue to work.
  • in non-trivial apps, it possible to have both side-loading and non-sideloaded versions of the same relationships (depending on how the user navigates the app)
  • Internally, relationships will no longer have bifurcating code paths to handle async and sync, this should be a solid improvement.

the con:

  • more then's in the codebase

Solution to the con:

Provide abstractions that remove the thens. In the future, we will have yield which helps dramatically, but until then (potentially still to augment what we will have then)
the following would be great.

 // this is relationship and this works today
User.find(1).get('favoriteBook')

// a nested relationship, and this does not work correctly today
User.find(1).get('favoriteBook.author') 

// one must
User.find(1).get('favoriteBook').then(function(favoriteBook) {
  return favoriteBook.get('author')
});

// proposal
// support nested relationships in the PromiseProxy.

User.find(1).get('favoriteBook.author').then(function(author) {
  // no intermediate then is needed.
});

Crazy future spike/idea/thing:

In theory, both gets and sets could be supported on promise proxies. Which could make them appear transparent. The caveat is, its complicated, order is hard, and conflict resolution is hard.
See: http://www.erights.org/talks/thesis/ for more details on this

@tim-evans
Copy link
Contributor

This would be fantastic- I've bumped into this and confused me. I think first and foremost, it would be nice to just have polymorphic async relations working.

@jherdman
Copy link

Does this effectively resolve #1535?

@igorT
Copy link
Member

igorT commented Jan 6, 2015

Should be fixed by #2623

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.

8 participants