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

ModelSync.Local implementation (final) - passing on A-grade browsers #1218

Merged
merged 17 commits into from Sep 27, 2013

Conversation

Projects
None yet
3 participants
@clarle
Copy link
Contributor

clarle commented Sep 23, 2013

This implements the feature discussed in #1156.

The previous problem with the PR in #385 was that there were failing tests on older browsers (IE6 and IE7) that did not support localStorage. This includes the fixes for those failing tests, which now pass successfully and fallback to the internal cache.

As mentioned before, ModelSync.Local is currently being used on the YUI TodoMVC and has no other problems beyond the fallback in the older browsers.

Error handling is included with ModelSync.Local to handle the case when localStorage passes its maximum allocated space.

} catch (e) {
return false;
}
})(),

This comment has been minimized.

@ezequiel

ezequiel Sep 23, 2013

Contributor

Perhaps we should advocate the use of feature testing, rather than simply detecting whether localStorage exists or not. For example, we should test if we're actually able to set and remove items from localStorage, like so:

if (localStorage is detected and the following pass):
    localStorage.setItem(..., ...);
    localStorage.removeItem(...);
then localStorage is available and useable

The main reason being there are times when localStorage is detected, but is actually unusable. For example, when browsing in private mode on an iOS device, localStorage is available but cannot be used.

This comment has been minimized.

@clarle

clarle Sep 23, 2013

Contributor

This sounds good to me, I'll push this up in just a bit.


config || (config = {});

if ('root' in config) {

This comment has been minimized.

@ezequiel

ezequiel Sep 23, 2013

Contributor

I suppose this assumes a root property may not ever exist in config's prototype chain, which is a fair assumption. However, I believe it's the wrong kind of test in this case.

config.hasOwnProperty("root");

seems more correct. Then again, since root is expected to be a string of length greater than one:

if (config.root) { ... }

suffices.

This comment has been minimized.

@clarle

clarle Sep 23, 2013

Contributor

Good point, but I think we need to check the prototype, since this is a mix-in for a Y.Base class. In that case, if something along the prototype chain has a root property, then it should use that.

This comment has been minimized.

@ezequiel

ezequiel Sep 25, 2013

Contributor

I see. in is the correct choice, then.

config || (config = {});

if ('root' in config) {
this.root = config.root || '';

This comment has been minimized.

@ezequiel

ezequiel Sep 23, 2013

Contributor

Since root already defaults to an empty string, having || '' is unnecessary. It can be safely removed.

This comment has been minimized.

@clarle

clarle Sep 24, 2013

Contributor

I guess this might protect against the case where someone actually does explicitly specify '' in their root, which would return falsey.

This comment has been minimized.

@ericf

ericf Sep 25, 2013

Member

I like this pattern of coercing all falsy values to an empty string. This way the rest of the code can always assume root is a string, even if the person had {root: false}.

} else {
callback("Data not found in LocalStorage");
}
}

This comment has been minimized.

@ezequiel

ezequiel Sep 23, 2013

Contributor

The above is logically equivalent to the following simplified version:

if (response) {
    callback(null, response);
} else if (errorInfo) {
    callback(errorInfo);
} else {
    callback("Data not found in LocalStage.");
}
@property _NON_ATTRS_CFG
@type Array
@default ['root'']

This comment has been minimized.

@ezequiel

ezequiel Sep 23, 2013

Contributor

['root']

@param {callback} [callback] Called when the sync operation finishes.
@param {Error|null} callback.err If an error occurred, this parameter will
contain the error. If the sync operation succeeded, _err_ will be
falsy.

This comment has been minimized.

@ezequiel

ezequiel Sep 23, 2013

Contributor

falsey

} catch (e) {
return false;
}
})();

This comment has been minimized.

@ezequiel

ezequiel Sep 24, 2013

Contributor

The advice given earlier on feature testing should apply here as well. Also, Y.config.win.


model._save = function () {
throw new Error('Failed sync');
}

This comment has been minimized.

@ezequiel

ezequiel Sep 24, 2013

Contributor

Don't forget your semicolons.

delete Y.TestModel;
delete Y.TestModelList;
try {
Y.config.win.storage.clear();

This comment has been minimized.

@ezequiel

ezequiel Sep 24, 2013

Contributor

Did you mean Y.config.win.localStorage, or did you actually create a global variable named storage?

Clarence Leung
Add changes based on @ezequiel's suggestions
* Switched to feature testing, rather than feature detection
* Various fixes to improve code clarity
@clarle

This comment has been minimized.

Copy link
Contributor

clarle commented Sep 24, 2013

@ezequiel Thanks for the feedback and attention to detail!

Made the changes like you suggested, let me know what you think now!

this.root = config.root || '';
}

if (this.model && this.model.prototype.root) {

This comment has been minimized.

@ericf

ericf Sep 25, 2013

Member

Can you add a code comment there that this is checking if the sync layer is being applied to a ModelList and if so, trying to look for a root on its model's propto.

}

if (this.model && this.model.prototype.root) {
this.root = this.model.prototype.root;

This comment has been minimized.

@ericf

ericf Sep 25, 2013

Member

config.root should trump a ModelList's model's root. I'd add an !this.root to the if statement.

// Pull in existing data from localStorage, if possible.
// Otherwise, see if there's existing data on the local cache.
if (store) {
LocalSync._data[this.root] = Y.JSON.parse(store);

This comment has been minimized.

@ericf

ericf Sep 25, 2013

Member

Do you want to try/catch around Y.JSON.parse() since it can throw?

if (store) {
LocalSync._data[this.root] = Y.JSON.parse(store);
} else {
LocalSync._data[this.root] = (LocalSync._data[this.root] || {});

This comment has been minimized.

@ericf

ericf Sep 25, 2013

Member

Could use conditional assignment here and swap the = and || in this line. Just a style thing…

Or use an else if

if (this._isYUIModelList) {
response = this._index(options);
} else {
response = this._show(options);

This comment has been minimized.

@ericf

ericf Sep 25, 2013

Member

What about switching _show to _read? Or did you do this to disambiguate "read" since it's different for models and lists?

This comment has been minimized.

@clarle

clarle Sep 25, 2013

Contributor

Yeah, that was the main decision behind that. I could switch it to _read if it's more clear though.

This comment has been minimized.

@ericf

ericf Sep 25, 2013

Member

Either way it's fine, it's a protected helper method so since you explicitly called it _show() for a reason, I won't bikeshed it.

@since @VERSION@
**/
_update: function (options) {
var hash = Y.merge(this.toJSON(), options);

This comment has been minimized.

@ericf

ericf Sep 25, 2013

Member

Why are you merging in the options here?

if (LocalSync._hasLocalStorage) {
this.storage && this.storage.setItem(
this.root,
Y.JSON.stringify(LocalSync._data[this.root])

This comment has been minimized.

@ericf

ericf Sep 25, 2013

Member

I'm curious to know why you're serializing all the objects under this root on every save, instead of only serializing the one model or the entire list on save.

This comment has been minimized.

@clarle

clarle Sep 25, 2013

Contributor

localStorage is an un-nested string-based key/value mapping. In the system here (and with the TodoMVC spec), each Model/ModelList root provides a single key, and the entire class of objects it represents needs to be stringified into one value.

You could serialize all of the objects into individual key/value pairs, but that would be less performant once you have to access all of the objects at once, which is a pretty common use case.

This comment has been minimized.

@ericf

ericf Sep 25, 2013

Member

Okay cool, seems like the right tradeoff to keep the state for the whole root in memory to make loads/reads faster and you have do deal with the key -> string limitation anyways.

What's happens when there's several classes and instances that use the same root, are they both pointing to the same structure in memory, is there a situation where they overwriting each other's operations?

var store;

try {
store = Y.config.win.localStorage;

This comment has been minimized.

@ericf

ericf Sep 25, 2013

Member

I think this data should be set back after this suite is done running.

@ericf

This comment has been minimized.

Copy link
Member

ericf commented Sep 25, 2013

@clarle this is looking good; I added some line comments. What sort of coverage numbers does this have?

@ericf

This comment has been minimized.

Copy link
Member

ericf commented Sep 25, 2013

How are you dealing with a model list which wants its root data as an array, and model which wants the root as an object to be able to lookup its data by id?

I think we may need to rethink the storage structure, unless it's acceptable for loading a model list from localStorage to not guarantee order.

@since @VERSION@
**/
_index: function (options) {
return Y.Object.values(LocalSync._data[this.root]);

This comment has been minimized.

@ericf

ericf Sep 25, 2013

Member

This is effectively saying that storage order is not significant for a model list's data. If we want to make it significant and consistent, then I don't want us to store it as an object whose key-order is not guaranteed, especially when going to/from JSON.

Clarence Leung added some commits Sep 26, 2013

Clarence Leung
Update to new `localStorage` mechanism
This new mechanism now stores data in the folllowing way:

Key   : 'users'
Value : [{id: 'users-1', name: 'clarle'}, {id: 'users-2}, name: 'ericf'}]

This guarantees the order of the values. A in-memory lookup table by `id` is
used to do lookups faster than iterating through the entire array.
Clarence Leung
Add suggestions from @ericf on ModelSync.Local
* Add try/catch to JSON.parse
* Add a few clarifying comments on `initializer`
* Using clearer conditional assignment style
@clarle

This comment has been minimized.

Copy link
Contributor

clarle commented Sep 26, 2013

@ericf Done - I've changed the storage mechanism to the following.

Key : 'users'
Value : [{id: 'users-1', name: 'clarle'}, {id: 'users-2, name: 'ericf'}]

An in-memory lookup table that maps id to the specific object in memory is also used for doing faster loads for a single Model.

I've also made all the changes that you mentioned, so let me know if there are any other pressing issues.

@clarle

This comment has been minimized.

Copy link
Contributor

clarle commented Sep 26, 2013

Coverage stats:

--------------------------+-----------+-----------+-----------+-----------+
File                      |   % Stmts |% Branches |   % Funcs |   % Lines |
--------------------------+-----------+-----------+-----------+-----------+
model-sync-local/         |     97.14 |     87.18 |       100 |     97.14 |
      model-sync-local.js |     97.14 |     87.18 |       100 |     97.14 |

Not all branches will be reachable since some are the fallback branches, so I think this should be fine.

@ericf

This comment has been minimized.

Copy link
Member

ericf commented Sep 27, 2013

@clarle could you add tests for the serialization order, mainly to make sure there won't be future regression on that. Then merge this!

@clarle clarle merged commit 2797cd8 into yui:dev-3.x Sep 27, 2013

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