Records don't always stay deleted #1115

Closed
wagenet opened this Issue Aug 13, 2013 · 18 comments

Comments

Projects
None yet
8 participants
Owner

wagenet commented Aug 13, 2013

http://emberjs.com/guides/getting-started/using-other-adapters/

If you flag an issue as completed, then hit the "Clear Completed" button, then click active, then all you will see the previously cleared issue re-appear. It does seem to save to the local storage correctly however because if you refresh the page it should stay deleted.

via @cr125rider in #962

wycats was assigned Aug 13, 2013

Owner

wycats commented Sep 1, 2013

@wagenet is this still true in 1.0 Beta 1?

Contributor

balinterdi commented Sep 2, 2013

FYI I'm updating the ember.js example in todomvc. Once I get to the point where the app works, I can verify if the above still holds with 1.0.Beta 2.

Owner

stefanpenner commented Sep 2, 2013

zombies

Contributor

balinterdi commented Sep 2, 2013

Contributor

balinterdi commented Sep 2, 2013

@stefanpenner Oh, sorry, I get it :) Thought you replied to my last comment.

Owner

wycats commented Sep 2, 2013

Can someone try to identify the root cause?

Contributor

balinterdi commented Sep 3, 2013

I verified and this bug is not present when using 1.0 Beta 1. Deleted todos stay deleted.

Contributor

sly7-7 commented Sep 3, 2013

@wycats I tried to reproduce it, but I can't with the last version provided in the guides. So I guess this one is resolved

Unfortunately, I hit another one.

  • double click on a todo
  • hit backspace to make it empty
  • hit enter

This cause the todo to be removed, but then the sequence
todo.deleteRecord();
todo.save();

ends with:

Uncaught Error: Attempted to handle event `willCommit` on <Todos.Todo:ember298:f09gn> while in state root.deleted.saved.  ember-data.js:3387
DS.Model.Ember.Object.extend._unhandledEvent ember-data.js:3387
DS.Model.Ember.Object.extend.send ember-data.js:3337
DS.Model.Ember.Object.extend.adapterWillCommit ember-data.js:3436
DS.Store.Ember.Object.extend.scheduleSave ember-data.js:1961
DS.Model.Ember.Object.extend.save ember-data.js:3586
Todos.TodoController.Ember.ObjectController.extend.actions.removeTodo runner:89

Also reproduced on ember-data-latest

Contributor

balinterdi commented Sep 3, 2013

@sly7-7 I could not reproduce what you report using the updated todomvc version.

When I delete the title of the todo and hit enter, it updates it to the empty string and no error is thrown.

Contributor

sly7-7 commented Sep 3, 2013

@balinterdi I was just playing with the jsbin included in http://emberjs.com/guides/getting-started/using-other-adapters/
I just try it, and I encounter it...

Contributor

balinterdi commented Sep 3, 2013

I checked and there is in fact a difference between the todomvc version and the version in the getting-started guide in regards of how they handle the case when a todo with an empty title is “submitted”. The todomvc version saves it and the getting-started guide tries to remove it.

So if we consider the todomvc version the authoritative one then we should just copy the behavior from it to the guide version.

I can do that if someone more involved confirms that this is the right way to go.

Member

bantic commented Sep 3, 2013

@balinterdi @sly7-7 I'd be happy to update the guide to more closely match the todomvc version (or vice versa).

Right now there are three semi-authoritative versions of the app that are all a little different:

  • at the todomvc site (currently at ember version rc1 but hopefully at 1.0 soon when the PR gets merged)
  • as described in the code samples of the getting started guide pages, and linked to in the "diff of this change" links at the getting started guide (those are diffs of changesets at the quickstart-code-sample repo ).
  • the jsbins that are linked to from the getting started guide

I updated the todomvc to be at 1.0 by:

  • continuing on the work that was done by wycats last week ( diff ) which is based pretty closely on the existing todomvc app (ember rc1)
  • updating the vendored libs to ember 1.0.0 and ember data 1.0.0-beta.1
  • refactoring the code to match the code in the getting started guide as closely as I could (there's only one real change, where I use .on('didInsertElement') in the view because that felt more idiomatic to v1.0.0)
  • I noticed the error when clearing out a todo's text, but since this behavior isn't mentioned in the getting started guide (and I couldn't make it work without some slight hackitude) I removed it from the code. In my PR if you remove the text from a todo and hit enter you get a todo with no text.
Member

bantic commented Sep 3, 2013

Also, as far as I see the getting started guide doesn't actually include code that attempts to remove an empty todo in its linked repo or in its text. That's only in the linked jsbins. (And they work up until the point where the adapter is changed from the fixture adapter to the LSAdapter)

Contributor

balinterdi commented Sep 3, 2013

So what about we rewrite the code in the jsbin to just let the todo's title be updated to an empty string (as in your PR)? This way, we'll have working code there and less divergence between the semi-authoritative versions.

Member

bantic commented Sep 5, 2013

Yep, I'd be happy to do that. I am not sure who is the correct person to
make that call, though. If the core folks are on board with that change I
can submit a PR to change the getting started guide on the website
appropriately.

On Tue, Sep 3, 2013 at 3:10 PM, Balint Erdi notifications@github.comwrote:

So what about we rewrite the code in the jsbin to just let the todo's
title be updated to an empty string (as in your PR)? This way, we'll have
working code there and less divergence between the semi-authoritative
versions.


Reply to this email directly or view it on GitHubhttps://github.com/emberjs/data/issues/1115#issuecomment-23738716
.

http://twitter.com/bantic

Contributor

knownasilya commented Feb 19, 2014

Attempted to handle event willCommit on Todos.Todo:ember298:f09gn while in state root.deleted.saved

@sly7-7 did you find a solution to the above error? I'm finding this same error in 1.0.0-beta.6.

I do createRecord which is later followed by a destroyRecord and get this same error. Probably need a better error.

Contributor

sly7-7 commented Feb 20, 2014

@knownasilya Unfortunately I did not retry this kind of thing. But what you say seems like a bug. Perhaps you could open an other issue, with a jsbin/jsfiddle or a test demonstrating this.

Owner

igorT commented May 28, 2014

The error was fixed by #1671

igorT closed this May 28, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment