getoutreach / epf

A framework for keeping your Ember.js apps in sync.
http://epf.io
MIT License
369 stars 33 forks source link

Tackle Child Session problems #59

Closed taras closed 10 years ago

taras commented 11 years ago

Hi @ghempton,

I selected EPF as my persistence layer after using Ember Data and Ember Model. I've done some work to integrate it with my backend #48 but now I'm bumping into issues related to Child Sessions that are described in #22 and #49. I was hoping to use EPF during the Toronto Music Hack Day but I'll probably need this functionality working.

If you don't have plans to address this before the weekend, would you mind if I took a crack at it?

I already cloned the project and downloaded the code. My plan would be to start off by writing failing tests that cover the cases that need to be addressed. I looked at the tests and I can see that they're written in EmberScript. Do I need to learn EmberScript to write these tests? or can I just write regular JS?

Is this something that someone who is new to EPF can try to tackle in a few days or does it require your attention?

What do you think?

Taras

ghempton commented 11 years ago

I think the biggest blocker for implementing this is actually nailing down exactly what behavior we want out of the three options. I think creating the test cases first is a good approach.

I would like to keep the tests written in EmberScript. It's a pretty easy language to pick up: you can essentially think of it as CoffeeScript with no need to write get or set anymore.

As the child session functionality you mentioned only really affects how you might handle errors, do you think there is a way to work around it for the hack day? I am happy helping you in any way.

P.S. feel free to add me to gchat: ghempton@gmail.com

taras commented 11 years ago

Yeah, I think that it would be better to be thoughtful about this and not rush it unnecessarily. I would still like to attempt to address this issue. There are several issues that are mixed in here and maybe we can flush them out via tests before writing the code.

In #49, you mentioned the 3 different kinds of behaviour.

  1. Wait until the child session flush() is successful before updating the parent.
  2. Optimistically update the parent when there is a flush() and rollback if it fails.
  3. Keep the existing behavior and assume there is only isolation up until the first flush() call.

From the comments, option 1 seems to be prefered but that might be because its easiest to understand.

There are 2 things that are not clear to me:

  1. In option 2, you mention "optimistically" updating parents. You also mentioned this several times in the video. I get a sense that you see this as an important concept but I haven't seen it explained anywhere. Could you elaborate a bit more about the importance of this attitude towards managing persistence?
  2. In summary, could you explain what the existing behaviour is? and what's broken vs. misunderstood?
ghempton commented 11 years ago

Sorry for not getting back to you in a timely fashion (been traveling home from a vacation).

  1. Optimistically updating simply means that the client will "assume" that the server request will succeed without error (e.g. no spinner). In the event that there is an error, there could be an error dialog that redirects them to the failed form (or something even more generic like gmails "there was an error bar at the top of the application". In the case of some part of the application that is likely to fail (e.g. username selection) then you probably would not want to make this assumption (hence maybe multiple isolation levels).
  2. The current behavior today:
    1. Once childSession.flush() is called, the parent is updated immediately.
    2. childSession.flush() still returns a promise that does not resolve until the server has returned.
    3. The results from the flush are merged against the parent and then the child merges in that result.
    4. In the case of an error, the errors must be dealt with in the parent session.

Any more thoughts?

taras commented 11 years ago

I took a day off yesterday to recover from the Hackathon.

Thank you for the description of the optimistic updating.

I'm going through the code today, I want to get clear on what the code says is happening to get an idea of what tests to write. I'm going to post later today with an update.

taras commented 11 years ago

Ok, I've done through all of the flush related code in ChildSession, Session and RestAdapter.

I'm starting to get a better picture of what's happening with child sessions.

The promise of child sessions is to isolate a group of models from the parent session. There are 2 purposes for isolation:

  1. In Runtime: allows 1 or more models to be rolled back to their original state in runtime ( for example, after user clicks cancel or a form )
  2. For Persistence: to allow 1 or more models to be persisted separately from the parent session

The current implementation isolates models in runtime but merges them into the parent session during flush of the child sessions.

To wait until the child session flush() is successful before updating the parent is attractive because it makes child session behaviour consistent in runtime and during persistence. However, during persistence, it will not behave optimistically because it will have to wait for the response to return before updating the parent session.

Optimistically update the parent when there is a flush() and rollback if it fails is optimistic and the behaviour is consistent for both "in runtime" and "during persistence" operations. Its more complicated, but it might be necessary complexity.

Is the above correct?

taras commented 11 years ago

ping.

I just want to figure out what approach we're going to move forward with so I can start writing tests.

Let me know what you think.

matthooks commented 11 years ago

Is there any reason we wouldn't want the option to do either?

If I had to choose, I'd probably pick non-optimistic flushes.

lastobelus commented 10 years ago

@taras @ghempton do you still plan to work on this?

taras commented 10 years ago

@lastobelus no, its not a priority for me anymore because I don't work with EPF anymore.

ghempton commented 10 years ago

@lastobelus this isn't a priority for me currently since our app is built around the existing behavior. That said, I am all for implementing the ideal behavior.

lastobelus commented 10 years ago

@ghempton my current project is drawing to a close. However if a subsequent project has "always-saved" as a requirement as the current one did, this is something I would be quite interested in working on. In the end in my current app I had to make my own shadow object and managing the merging of a successful save with new user input. I think it would also be very useful to have some well-tested standard ways of doing debouncing/throttling at the adapter level.

ghempton commented 10 years ago

These semantics have finally been ironed out. As of now, by default childSession.flush will not update the parent until the flush has completed successfully (this corresponds to option 1 above). If you want the original behavior of optimistically updating the parent, you can use childSession.flushIntoParent(). Cheers!