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

v1.0.0-beta.16.1 (and canary) - error after willDestroy #2988

Closed
svox1 opened this issue Apr 11, 2015 · 10 comments
Closed

v1.0.0-beta.16.1 (and canary) - error after willDestroy #2988

svox1 opened this issue Apr 11, 2015 · 10 comments

Comments

@svox1
Copy link
Contributor

svox1 commented Apr 11, 2015

Ember-cli 0.2.3
Ember 1.12.0-beta.1

With v1.0.0-beta.15 everything works finde.

With v1.0.0-beta.16.1 I get an error after call

this.store.willDestroy()

If I try to make a request:

this.store.find("user", 1);

I get the error:

Uncaught TypeError: Cannot read property 'adapter:user' of undefinedstore.js:1901 ember$data$lib$system$store$$Service.extend.retrieveManagedInstancestore.js:1914 ember$data$lib$system$store$$Service.extend.lookupAdapterstore.js:1831 ember$data$lib$system$store$$Service.extend.adapterForstore.js:702 ember$data$lib$system$store$$Service.extend._flushPendingFetchForTypeember.debug.js:13590 Map.forEach.cbember.debug.js:13387 OrderedSet.forEachember.debug.js:13598 Map.forEachstore.js:696 ember$data$lib$system$store$$Service.extend.flushAllPendingFetchesember.debug.js:878 Queue.invokeember.debug.js:943 Queue.flushember.debug.js:748 DeferredActionQueues.flushember.debug.js:173 Backburner.endember.debug.js:576 (anonymous function)

@igorT
Copy link
Member

igorT commented Apr 12, 2015

You are trying to find something after you destroyed the store? It seems ok that it errors out. Why are you trying to do this?

@igorT igorT closed this as completed Apr 12, 2015
@svox1
Copy link
Contributor Author

svox1 commented Apr 12, 2015

I use this.store.willDestroy() to logged out current user. - to clean all models

After this I use the Login for a new user and get this error (I call this.store.find('user') after login).

With v1.0.0-beta.15 this works as expected.

@igorT
Copy link
Member

igorT commented Apr 12, 2015

this.store.willDestroy is meant to be called once the store is being destroyed, it's not a public method. What you probably want is store.unloadAll
Can you try that?

@svox1
Copy link
Contributor Author

svox1 commented Apr 12, 2015

Thanks for the quick response.

If I change it to unloadAll I get this error:

Uncaught TypeError: Cannot read property 'typeKey' of undefinedstore.js:1462 
ember$data$lib$system$store$$Service.extend.model  Forstore.js:1062 
ember$data$lib$system$store$$Service.extend.unloadAll  auth-manager.js:185 
logout  application.js:180 
logout  ember.debug.js:23670 
triggerEvent  ember.debug.js:44818 
trigger  ember.debug.js:43542 
Router.trigger  ember.debug.js:23165 
EmberObject.default.extend.send   ember.debug.js:26854 
mixin.Mixin.create.send   ember.debug.js:18354 
runRegisteredAction    ember.debug.js:224 
Backburner.run   ember.debug.js:15813 
run   ember.debug.js:18352 
ActionHelper.registerAction.ActionManager.default.registeredActions.(anonymous function).handler   ember.debug.js:36563 
(anonymous function)   jquery.js:4430 
jQuery.event.dispatch   jquery.js:4116 
jQuery.event.add.elemData.handle

In auth-manager 185 is this:
this.store.unloadAll();

@wecc
Copy link
Contributor

wecc commented Apr 13, 2015

@svox1 You have to provide a type to the unloadAll method. See http://emberjs.com/api/data/classes/DS.Store.html#method_unloadAll

@svox1
Copy link
Contributor Author

svox1 commented Apr 13, 2015

Ah sorry for that... that was the reason why I have study the ember-data code and used willDestroy because I didnt want to manually remove each model.

Here I was asking this question:
"Or is this method not for public use?" ;)
#2914

What a pity that this doesnt work anymore with v1.0.0-beta.16.1

@svox1
Copy link
Contributor Author

svox1 commented Apr 13, 2015

I found a "fix".

Can you please change the line:
https://github.com/emberjs/data/blob/v1.0.0-beta.16.1/packages/ember-data/lib/system/store.js#L1933
(in willDestory)
From:

delete this._containerCache;

To:

this._containerCache = Ember.create(null);

After that, everything is clean and I can use willDestroy();
Its a win win situation ;)

Update
I have create my first pull request to ember-data:
#2995 🎉 🎉

svox1 added a commit to svox1/ember-data that referenced this issue Apr 13, 2015
@igorT
Copy link
Member

igorT commented Apr 13, 2015

I think instead of messing with willDestroy, we should provide a method that clears all data, which iterates over the types and calls unloadAll. That method should then be called by willDestroy. @svox1 could you make a PR that does that instead?

@svox1
Copy link
Contributor Author

svox1 commented Apr 13, 2015

Gladly, but I doesn't know enough about the internal ember-data handling.
Whats is recordArrayManager?

 this.recordArrayManager.destroy();

and the containerCache?

   for (var cacheKey in this._containerCache) {
      this._containerCache[cacheKey].destroy();
      delete this._containerCache[cacheKey];
    }

    delete this._containerCache;

Maybe this is enough:

// reset or better clear or unloadAll and make parameter 'type' optional because I think unloadAll is the right name for this?
reset: function() {
    var typeMaps = this.typeMaps;
    var keys = Ember.keys(typeMaps);

    var types = map(keys, byType);

    forEach(types, this.unloadAll, this);

    function byType(entry) {
      return typeMaps[entry]['type'];
    }
  },

And maybe we can call this.reset(); in willDestroy(); if the order of the call this.recordArrayManager.destroy(); is not important.

Sorry Im not familiar with the ember-data code... Iam only a consumer ;)

@wecc
Copy link
Contributor

wecc commented Apr 13, 2015

You're on the right track! Move the parts responsible for unloading all types to a separate function (reset in your example) and call that method from willDestroy, after the call to this.recordArrayManager.destroy().

I agree that unloadAll is a better suited name for this method and that we should probably rename the current unloadAll to unloadType. We can check if a parameter is provided to unloadAll, show a deprecation warning and call unloadType if that is the case. If no parameter is provided to unloadAll, we proceed with unloading all types.

Feel free to ping me on IRC, I'd be happy to help.

svox1 added a commit to svox1/ember-data that referenced this issue Apr 13, 2015
change unloadAll behavior to: unload all data in the store
Related discussion emberjs#2988
svox1 added a commit to svox1/ember-data that referenced this issue Apr 13, 2015
svox1 added a commit to svox1/ember-data that referenced this issue Apr 13, 2015
change unloadAll behavior to: unload all data in the store
Related discussion emberjs#2988

Revert "fix issue emberjs#2988 (v1.0.0-beta.16.1 (and canary) - error after willDestroy)"

This reverts commit 2b599ad.
svox1 added a commit to svox1/ember-data that referenced this issue Apr 13, 2015
svox1 added a commit to svox1/ember-data that referenced this issue Apr 13, 2015
change unloadAll behavior to: unload all data in the store
Related discussion emberjs#2988

Revert "fix issue emberjs#2988 (v1.0.0-beta.16.1 (and canary) - error after willDestroy)"

This reverts commit 2b599ad.
svox1 added a commit to svox1/ember-data that referenced this issue Apr 13, 2015
change unloadAll behavior to: unload all data in the store
Related discussion emberjs#2988

Revert "fix issue emberjs#2988 (v1.0.0-beta.16.1 (and canary) - error after willDestroy)"

This reverts commit 2b599ad.
svox1 added a commit to svox1/ember-data that referenced this issue Apr 13, 2015
change unloadAll behavior to: unload all data in the store
Related discussion emberjs#2988
svox1 added a commit to svox1/ember-data that referenced this issue Apr 13, 2015
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

No branches or pull requests

3 participants