Exception when cache enabled: Uncaught (in promise) TypeError: this.map is not a function

[ptgamr]

If I disable cache caching: 'none' then I dont' get the following error:

DEBUG: -------------------------------
ember.debug.js:5938 DEBUG: Ember      : 2.2.0
ember.debug.js:5938 DEBUG: Ember Data : 2.1.0
ember.debug.js:5938 DEBUG: jQuery     : 1.11.3
ember.debug.js:5938 DEBUG: -------------------------------
VM7047:87 Ember Inspector Active
ember.debug.js:36601 Uncaught (in promise) TypeError: this.map is not a function(…)_emberMetalMixin.Mixin.create.copy @ ember.debug.js:36601copy @ ember.debug.js:30630set @ cache.js:22(anonymous function) @ localforage.js:228

[frederikbosch]

@ptgamr Is that with saving or requesting data? What method on the store fails? This way I create a test, reproduce, fix and release!

[ptgamr]

@frederikbosch this happen on both on loading record & persist record. The first time when the route is load with findAll: https://github.com/ptgamr/ember-offline-sync/blob/master/tests/dummy/app/routes/todos.js

You can run the dummy test app here: https://github.com/ptgamr/ember-offline-sync && comment out cache: 'none' here https://github.com/ptgamr/ember-offline-sync/blob/master/tests/dummy/app/adapters/application.js

[frederikbosch]

@ptgamr I cannot replicate this with a test. What I did was the following.

git clone https://github.com/genkgo/ember-localforage-adapter
npm install && bower install

Then modify tests/dummy/app/adapter/application.js to:

import LFAdapter from 'ember-localforage-adapter/adapters/localforage';

export default LFAdapter.extend({
  caching: 'none'
});

And then run:

ember test

Everything passes. No issues with 'this.map' is not a function. Moreover .map is not even found in the adapter nor serializer. I guess this an error in your own code.

[frederikbosch]

hmm, while upgrading to Ember / Ember Data 2.2.0 I hit the same issue. Will try to fix.

[frederikbosch]

@ptgamr Do you want to try if everything works with master branch? It requires Ember and Ember Data 2.2.x now.

[frederikbosch]

@ptgamr If you can confirm that this bug is fixed, I will release version 2.2.0 of the adapter.

@sebweaver Would you be so kind to review my changes? You have seen more Ember Data internals than I did. Below you will find an explanation why changes were made, and what specific were changed. I also upgraded to the latest Ember CLI and did an ember init on the project.

In the past we were making a copy of the data that was passed into the cache object using Ember.copy. Because in my experience, the same object was passed into the model. This upgraded the POJO data object to an Ember.Object. Furthermore, when one would change a model property, one would also change the cache item. This is not desired behaviour. Cache should basically be immutable.

Now with Ember 2.2.0, Ember.copy became an abstract method in the mixin Ember.Copyable while it was a static method in the past. Ember.A does not implement Ember.Copyable, so it must be removed from the cache object, which I did. So, the question is, are will seeing the desired behaviour, and not the behaviour as described in the previous paragraph? I tested that in a new test. It makes sure that cache remains a POJO and is not mutated when a model gets mutated.

[sebweaver]

@frederikbosch I'll check this out ASAP.

[ptgamr]

@frederikbosch thanks I'll check it

[sebweaver]

@frederikbosch Ok, I read this issue and reviewed your changes. There are several things to say.

About this issue

Ember.copy is not the culprit. It works as always: it calls copy on Ember.Copyable objects or applies its own implementation, including deep mode.

The Ember.NativeArray returned by Ember.A is not the culprit either. Its extends Ember.Copyable and implements copy.

The culprit was however the object you gave as argument to Ember.A. Its purpose is to upgrade Array like objects to Ember.NativeArray, but its behavior relies on the the value of EXTEND_PROTOTYPES:

• When EXTEND_PROTOTYPES is true, it does almost nothing and returns the given object as is.
• When EXTEND_PROTOTYPES is false, it actually upgrades the given object to a Ember.NativeArray.

Why is the latter related to this current issue? Because Ember.A assumes you give an Array like object which comes with its own native methods. Since the objects handled and stored by the ELA cache are not Array like, it breaks when Ember.NativeArray calls map on them during the copy!

So why did it work before an not anymore? Because EXTEND_PROTOTYPES which is true by default has been set to false somewhere.

What did that? Where? For @ptgamr's app I can't tell, it may be on purpose for some reasons. But for ELA it's related to the ember-disable-prototype-extensions which has been added during the migration to the latest Ember-CLI

Is it bad to disable prototype extensions? No, it's even a good practice for add-ons (like ELA). If you remove that extension with the old cache code, tests pass again (I've verified that) but it hides the real issue.

So what's the real problem in the end? The problem was to give non Array like objects to Ember.A in the first place.

Let's talk about that now.

About the cache and its changes

The previous cache code was flawed for 4 reasons:

1. Ember.A is not intended to work on non Array like objects and does nothing, at least, or upgrade to Ember.NativeArray, at most (which is problematic in our case, as seen above).
2. Ember.copy doesn't create Ember.Object from non Ember objects (i.e. POJOs) as you said. I don't know what made you think that. It just copies them "as is", with its default implementation.
3. Copying objects to/from the cache is unnecessary because they are already serialized from their respective models. Since serialization acts as a deep copy, changing attributes on these POJOs should not affect attributes of the models at all. Serialization is a hermetic layer between real objects handled by the adapter and Ember models handled by the store.
4. Even though copying objects would have been necessary (may be with some old versions of ED), copying twice (on set and get) was an overkill. If you already have a copy in the cache when you set, you don't need to copy again when you get.

Conclusion (TL;DR) :smile:

@frederikbosch Your changes:

1. Migrating to Ember 2.2
2. Migrating to the latest Ember-CLI
3. Disabling EXTEND_PROTOTYPES

..are great!

But changing the cache is better than great: it was necessary! At least for the sake of memory consumption (with copies of copies).

[frederikbosch]

@sebweaver :bowtie:! That is a phenomenal reply, I did not expect this! Thanks a lot. Things are more clear for me now. I did not even realize I added the dependency of ember-disable-prototype-extensions and what consequences it had.

What I am wondering is whether it is our job to disable EXTEND_PROTOTYPES. I mean the code now works with both true and false, right? If we are adding ember-disable-prototype-extensions to our package.json then version 2.2.0 of ELA becomes backward incompatible and breaks with semantic versioning. On the other side, it is not a bad idea to use best practises.

@sebweaver Do you think ember-disable-prototype-extensions should be included or should be it removed?

[frederikbosch]

Just learned on irc emberjs that I can release with ember-disable-prototype-extensions and I do not need to worry on backward compatible changes. Addons with Ember CLI have EXTEND_PROTOTYPES disabled by default. That is logical, otherwise there would have been no bug. So I will release ELA 2.2.0 today.

[ptgamr]

:+1: for you guys

[sebweaver]

This extension only disables EXTEND_PROTOTYPES for the current scope of the add-on. But the consuming application is not affected and can enable/disable at will.

That's why it's a good practice, because it makes the add-on code more generic and thus less error prone when embedded in an application which disables it.

So, yes, we should keep it! :+1:

[frederikbosch]

@sebweaver Thanks again, just released 2.2.0.