ModelSync.Local implementation (final) - passing on A-grade browsers #1218
Conversation
…o model-sync-local
Fixes bug with the fallback cache when localStorage is unavailable.
| } catch (e) { | ||
| return false; | ||
| } | ||
| })(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This sounds good to me, I'll push this up in just a bit.
* Switched to feature testing, rather than feature detection * Various fixes to improve code clarity
|
@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) { |
There was a problem hiding this comment.
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.
| var store; | ||
|
|
||
| try { | ||
| store = Y.config.win.localStorage; |
There was a problem hiding this comment.
I think this data should be set back after this suite is done running.
|
@clarle this is looking good; I added some line comments. What sort of coverage numbers does this have? |
|
How are you dealing with a model list which wants its I think we may need to rethink the storage structure, unless it's acceptable for loading a model list from |
| @since @VERSION@ | ||
| **/ | ||
| _index: function (options) { | ||
| return Y.Object.values(LocalSync._data[this.root]); |
There was a problem hiding this comment.
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.
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.
* Add try/catch to JSON.parse * Add a few clarifying comments on `initializer` * Using clearer conditional assignment style
|
@ericf Done - I've changed the storage mechanism to the following. Key : An in-memory lookup table that maps I've also made all the changes that you mentioned, so let me know if there are any other pressing issues. |
|
Coverage stats: Not all branches will be reachable since some are the fallback branches, so I think this should be fine. |
|
@clarle could you add tests for the serialization order, mainly to make sure there won't be future regression on that. Then merge this! |
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
localStoragepasses its maximum allocated space.