FIX: correctly fetch() content for Backbone.Model Objects #62

jDavidnet · 2013-01-13T00:58:58Z

This should now fix the scenario when Backbone.Model Objects are being used on their own and not as a model for a Backbone.Collection.

This also passes all tests now.

This lib did not correctly fetch()/ 'read'/'GET' from local storage for Backbone.Model objects. It does work in the tests however because the tests do not refresh the page and load once again from localStorage. in my personal test this worked for Backbone.Model, but I did not test Backbone.Collection Objects.

…mework test used 'length' as a property.

jDavidnet · 2013-01-13T01:01:44Z

FIX #49, #48: correctly fetch() content for Backbone.Model Objects

jeromegn · 2013-01-13T07:15:30Z

Can't be merged automatically. Could you rebase/merge master into your branch?

jeromegn · 2013-01-13T17:21:35Z

Never mind, I'll do it later this afternoon. I'm refactoring all tests and then some.

jeromegn · 2013-01-13T22:00:01Z

From my tests, this seems fine without your code. I did some logging in there and from the current test suite, the logging within your conditional never got called.

Can you write a failing test case with what's currently in master to prove your point? I fail to see the case where it doesn't work. (maybe a change since you opened this pull request fixed it by magic)

jDavidnet · 2013-01-15T02:07:36Z

Jerome, I will try to find the time to learn your testing frame work, but I
am busy at a startup trying to launch a product.

The case is this, if you have a model by it self it was not correctly
reading itself from local storage.

if you have a model as part of a collection, it would read it just fine.

For me to get back around to this will be a few days or longer if I have to
deal with fires at work as we are launching soon.

On Sun, Jan 13, 2013 at 2:00 PM, Jerome Gravel-Niquet <
notifications@github.com> wrote:

From my tests, this seems fine without your code. I did some logging in
there and from the current test suite, the logging within your conditional
never got called.

Can you write a failing test case with what's currently in master to prove
your point? I fail to see the case where it doesn't work. (maybe a change
since you opened this pull request fixed it by magic)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/62#issuecomment-12201099.

Justin Kruger
Social Media Software Engineer -
San Francisco, CA

http://jDavid.net
http://twitter.com/jdavid
http://www.linkedin.com/in/jdavid

jDavid.net@gmail.com

jeromegn · 2013-01-15T02:21:51Z

I completely get that :)

If you'd explain in more details what's wrong, I should be able to do it myself. Up to you.

jDavidnet · 2013-01-15T02:38:13Z

In your documentation you provide an example to make a Backbone.Collection
object as provided at the bottom of this email. You need a few tests based
on Backbone.Model. The Backbone.Collection works because it assumes the
data is an array, but for Backbone.Model Object when the object is not a
Collection, but a singleton Model, it was not working, it was locating the
wrong record in localStorage or not at all.

I hope that makes more sense.

Imagine the usage.

window.SomeModel = Backbone.Model.extend({

localStorage: new Backbone.LocalStorage("SomeCollection"), // Unique
name within your app.

// ... everything else is normal.
});

This is the code I am using.

AppSettings = Backbone.Model.extend({
defaults: {
notify: true,
notify_close_delay: 10, //seconds
notify_last_check: 0, //timestamp
notify_last_check2: 0 //timestamp
},
initialize:function(){
var that = this;
try{
// that.storageSync = new Backbone.StorageSync("AppSettings", that);
that.localStorage = new Backbone.LocalStorage("AppSettings",
that);

        that.on('change', function(){
            console.log('AppSettings - LOADED2', that, arguments);
        });
    }catch(e){
        console.log('Failed to init AppSettings.  LocalStorage Failed,

Using Defaults?')
}
}
});

https://github.com/jeromegn/Backbone.localStorage
Usage

Include Backbone.localStorage after having included Backbone.js:

type="text/javascript" src="backbone.localStorage.js"></script>

Create your collections like so:

window.SomeCollection = Backbone.Collection.extend({

localStorage: new Backbone.LocalStorage("SomeCollection"), // Unique
name within your app.

// ... everything else is normal.
});

On Mon, Jan 14, 2013 at 6:21 PM, Jerome Gravel-Niquet <
notifications@github.com> wrote:

I completely get that :)

If you'd explain in more details what's wrong, I should be able to do it
myself. Up to you.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/62#issuecomment-12250584.

Justin Kruger
Social Media Software Engineer -
San Francisco, CA

http://jDavid.net
http://twitter.com/jdavid
http://www.linkedin.com/in/jdavid

jDavid.net@gmail.com

jeromegn · 2013-01-15T02:42:58Z

That's what I thought it was. If that was the case, then the tests concerning the model would fail, no?

For instance, this line would not update the model correctly: https://github.com/jeromegn/Backbone.localStorage/blob/master/spec/localStorage_spec.js#L204

I ran those tests with your code, and the code under the conditional that checked for an Array was never called. Are you sure this is still an issue?

These are the models tests: https://github.com/jeromegn/Backbone.localStorage/blob/master/spec/localStorage_spec.js#L183-L238

jDavidnet · 2013-01-15T03:15:13Z

those tests only do a save/fetch pattern and test. your other tests had
more variations. what if you save/get? set/save/get? set/get?

you shouldn't be required to do a save and fetch to use the Model. the
model should always be in a valid state.

I traced through the code in my use. I also wrote a chrome.storage.sync
adapter based on your work and the mod works there too.

Sent from my mobile

On Jan 14, 2013, at 6:43 PM, Jerome Gravel-Niquet notifications@github.com
wrote:

That's what I thought it was. If that was the case, then the tests
concerning the model would fail, no?

For instance, this line would not update the model correctly:
https://github.com/jeromegn/Backbone.localStorage/blob/master/spec/localStorage_spec.js#L204

I ran those tests with your code, and the code under the conditional that
checked for an Array was never called. Are you sure this is still an issue?

These are the models tests:
https://github.com/jeromegn/Backbone.localStorage/blob/master/spec/localStorage_spec.js#L183-L238

—
Reply to this email directly or view it on
GitHubhttps://github.com//pull/62#issuecomment-12251039.

jeromegn · 2013-01-15T03:37:11Z

I'm not testing set and get because those are handled by Backbone.

We're not testing Backbone here. My library doesn't touch that part.

I do a fetch so that my plugin is being used.

I'll add some more tests and see if I can replicate the issue. It does look like you're the only one who reported this though.

Jerome Gravel-Niquet
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Monday, 14 January, 2013 at 22:15, Justin Kruger wrote:

those tests only do a save/fetch pattern and test. your other tests had
more variations. what if you save/get? set/save/get? set/get?

you shouldn't be required to do a save and fetch to use the Model. the
model should always be in a valid state.

I traced through the code in my use. I also wrote a chrome.storage.sync
adapter based on your work and the mod works there too.

Sent from my mobile

On Jan 14, 2013, at 6:43 PM, Jerome Gravel-Niquet notifications@github.com
wrote:

That's what I thought it was. If that was the case, then the tests
concerning the model would fail, no?

For instance, this line would not update the model correctly:
https://github.com/jeromegn/Backbone.localStorage/blob/master/spec/localStorage_spec.js#L204

I ran those tests with your code, and the code under the conditional that
checked for an Array was never called. Are you sure this is still an issue?

These are the models tests:
https://github.com/jeromegn/Backbone.localStorage/blob/master/spec/localStorage_spec.js#L183-L238

—
Reply to this email directly or view it on
GitHubhttps://github.com//pull/62#issuecomment-12251039.

—
Reply to this email directly or view it on GitHub (#62 (comment)).

jDavidnet · 2013-01-15T06:41:18Z

how do i set up the new test environment

i have node installed, and i did the npm install for the two modules
globally, but i they won't work on the command line.

i have mac osx, could you point me to someplace to help me get this set up?

On Mon, Jan 14, 2013 at 7:37 PM, Jerome Gravel-Niquet <
notifications@github.com> wrote:

I'm not testing set and get because those are handled by Backbone.

We're not testing Backbone here. My library doesn't touch that part.

I do a fetch so that my plugin is being used.

I'll add some more tests and see if I can replicate the issue. It does
look like you're the only one who reported this though.

Jerome Gravel-Niquet
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Monday, 14 January, 2013 at 22:15, Justin Kruger wrote:

those tests only do a save/fetch pattern and test. your other tests had
more variations. what if you save/get? set/save/get? set/get?

you shouldn't be required to do a save and fetch to use the Model. the
model should always be in a valid state.

I traced through the code in my use. I also wrote a chrome.storage.sync
adapter based on your work and the mod works there too.

Sent from my mobile

On Jan 14, 2013, at 6:43 PM, Jerome Gravel-Niquet <
notifications@github.com>
wrote:

That's what I thought it was. If that was the case, then the tests
concerning the model would fail, no?

For instance, this line would not update the model correctly:

https://github.com/jeromegn/Backbone.localStorage/blob/master/spec/localStorage_spec.js#L204

I ran those tests with your code, and the code under the conditional
that
checked for an Array was never called. Are you sure this is still an
issue?

These are the models tests:

https://github.com/jeromegn/Backbone.localStorage/blob/master/spec/localStorage_spec.js#L183-L238

—
Reply to this email directly or view it on
GitHub<
https://github.com/jeromegn/Backbone.localStorage/pull/62#issuecomment-12251039>.

—
Reply to this email directly or view it on GitHub (
#62 (comment)).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/62#issuecomment-12252160.

Justin Kruger
Social Media Software Engineer -
San Francisco, CA

http://jDavid.net
http://twitter.com/jdavid
http://www.linkedin.com/in/jdavid

jDavid.net@gmail.com

jeromegn · 2013-01-15T15:02:06Z

The setup is a bit complicated, but you could just open the spec/runner.html in your browser. :) The require.js tests won't run, but the rest will.

Check your path, make sure your global and local node_modules are in there. For instance:

PATH="./node_modules/.bin:/usr/local/bin:/usr/local/sbin:$PATH"

You'll also need to install phantomjs. If you're on OS X, Homebrew is perfect for that.

jDavidnet and others added 3 commits November 13, 2012 17:11

added semicolon to test.

5d0f246

added an Array type/inheritance check before checking the length, fra…

8ad263d

…mework test used 'length' as a property.

jeromegn closed this Jan 13, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: correctly fetch() content for Backbone.Model Objects #62

FIX: correctly fetch() content for Backbone.Model Objects #62

jDavidnet commented Jan 13, 2013

jDavidnet commented Jan 13, 2013

jeromegn commented Jan 13, 2013

jeromegn commented Jan 13, 2013

jeromegn commented Jan 13, 2013

jDavidnet commented Jan 15, 2013

jeromegn commented Jan 15, 2013

jDavidnet commented Jan 15, 2013

jeromegn commented Jan 15, 2013

jDavidnet commented Jan 15, 2013

jeromegn commented Jan 15, 2013

jDavidnet commented Jan 15, 2013

jeromegn commented Jan 15, 2013

FIX: correctly fetch() content for Backbone.Model Objects #62

FIX: correctly fetch() content for Backbone.Model Objects #62

Conversation

jDavidnet commented Jan 13, 2013

jDavidnet commented Jan 13, 2013

jeromegn commented Jan 13, 2013

jeromegn commented Jan 13, 2013

jeromegn commented Jan 13, 2013

jDavidnet commented Jan 15, 2013

jeromegn commented Jan 15, 2013

jDavidnet commented Jan 15, 2013

jeromegn commented Jan 15, 2013

jDavidnet commented Jan 15, 2013

jeromegn commented Jan 15, 2013

jDavidnet commented Jan 15, 2013

jeromegn commented Jan 15, 2013