Skip to content
This repository has been archived by the owner on Dec 5, 2019. It is now read-only.

DataManager - Memory - initialize store to empty array - AGJS-105 #87

Closed
wants to merge 1 commit into from

Conversation

lfryc
Copy link
Contributor

@lfryc lfryc commented Dec 3, 2013

@lfryc
Copy link
Contributor Author

lfryc commented Dec 3, 2013

As this obviously fixes the issue and additionally it passes all tests,


I have considered initializing data to [] in DataManager base:
https://github.com/aerogear/aerogear-js/blob/master/src/data-manager/adapters/base.js#L28

That change makes some tests to fail (WebSQL) as there is some inheritance problem:
WebSQL calls Memory: https://github.com/aerogear/aerogear-js/blob/master/src/data-manager/adapters/websql.js#L455
but then it resolves to invocation of addDataRecord method
https://github.com/aerogear/aerogear-js/blob/master/src/data-manager/adapters/memory.js#L251
that doesn'e exist in inheritance chain of WebSQL datamanager:


Shouldn't we test all data manager implementations for their initial state?

@tolis-e
Copy link
Contributor

tolis-e commented Dec 4, 2013

@lfryc In case you initialize data to [] in Data manager base.js

https://github.com/aerogear/aerogear-js/blob/master/src/data-manager/adapters/base.js#L28

or modify the getData function:

https://github.com/aerogear/aerogear-js/blob/master/src/data-manager/adapters/base.js#L41

to return data || [];

then you also need to change line:

https://github.com/aerogear/aerogear-js/blob/master/src/data-manager/adapters/memory.js#L241

to if ( this.getData() && this.getData().length !== 0 ) {

and all the tests are passing. I don't see any inheritance problem.

To be honest, I don't like the initialization this.setData( [] ); here:

https://github.com/lfryc/aerogear-js/blob/ba7380bc6f6603203ab7ecf31857d0bde66c3822/src/data-manager/adapters/memory.js#L44 It seems like a hack

@lholmquist
Copy link
Contributor

i agree with @tolis-e that if we are going to do something, it should be to return data || [] in https://github.com/aerogear/aerogear-js/blob/master/src/data-manager/adapters/base.js#L41

and then update
https://github.com/aerogear/aerogear-js/blob/master/src/data-manager/adapters/memory.js#L241

to if ( this.getData() && this.getData().length !== 0 ) {

@lfryc
Copy link
Contributor Author

lfryc commented Dec 4, 2013

+1 updated pull request

@lholmquist
Copy link
Contributor

👍

@lholmquist
Copy link
Contributor

landed 7192abe

@lholmquist lholmquist closed this Dec 4, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants