fix: storeName can be used in createInstance and instance functions.

[hozkok]

createInstance({name: 'foo', storeName: 'bar'});
createInstance({name: 'foo', storeName: 'differentName'})

are now allowed.

instance('name') is deprecated but can still be used,
instance({name: 'foo', storeName: 'bar'}) is preferred now.

[scotttrinh]

Some of these cases are really tests for createInstance, which we should have, but don't. You're welcome to move them into their own describe block and write real test cases around createInstance since this is relevant to the PR. For testing instance, will you just move a few of these cases into a beforeEach to set up the state of the test, and then only it the things that describe the instance method? Like:

beforeEach(function () {
  testInstance_1 = $localForage.createInstance({
    name: 'testName',
    storeName: 'TEST_INSTANCE_1'
  });
  testInstance_2 = $localForage.createInstance({
    name: 'testName',
    storeName: 'TEST_INSTANCE_2'
  });
});

it('retrieve the instance by name and storeName', function () {
  expect($localForage.instance({
    name: 'testName',
    storeName: 'TEST_INSTANCE_1'
  })).toBe(jasmine.any(LocalForageInstance));
});

[scotttrinh]

@hozkok Can you also update the README.md to reflect the new behavior of instance and createInstance?

[hozkok]

@scotttrinh yep, it's done. I added tests and updated doc.

[scotttrinh]

I'll take a look at it tomorrow, thanks for all the work!

[scotttrinh]

Awesome work @hozkok , thanks again for the contribution! I have only a few minor nits to pick before I merge us into master. This will go out in the 1.4.0 release in a few weeks. See my comments inline.

[scotttrinh]

No need to exercise setItem and getItem here to ensure that they're different. A simple:

expect(instance1).not.toBe(instance2)

will suffice to show that they are two separate references since they are reference types, which will clean up the $q stuff below.

[hozkok]

Yea, they are the same LocalForageInstance references. but the createInstance behavior in localforage library drove me to write such tests because;
var inst1 = localforage.createInstance({name: 'foo', storeName: 'bar'});
var inst2 = localforage.createInstance({name: 'foo', storeName: 'bar'});
console.log('same ref:', inst1 === inst2);
// same ref: false

But you're right, in that case that is not a problem.

[scotttrinh]

Interesting. From my reading of the code you submitted, the second var would throw an error, is that right?

I would say as long as:

var fooBar = { name: 'foo', storeName: 'bar' };
expect($localForage.instance(fooBar)).toBe($localForage.instance(fooBar));

is true, let's just simplify the test case to make it a little easier to understand what instance does in isolation from the other methods.

Might even be worth adding that as a test case, just to show that we're passing the instance by reference and expect that instance is (somewhat) referentially transparent?

[hozkok]

I have already applied your suggestion and changed the test to not.toBe(instance).

my example was not for the $localForage, but for localforage itself.

in the localforage library, there is no instance function, just createInstance and my example just works fine (and two same instances are different references too.). I think in localforage module, createInstance name is a bit confusing, needs to be instance instead. because it also takes care of createInstance. in angular-localForage, the similar behavior can be produced with a function;

function instance(opts) {
    try {
        return $localForage.createInstance(opts);
    } catch(e) {
        return $localForage.instance(opts);
    }
}

[scotttrinh]

Yeah, I followed that example you posted. Thanks for the clarification!

[scotttrinh]

Thanks again, @hozkok , I'll make sure this gets in the 1.4.0 release!

[brunodeprez]

Hi,

this feature is very important for me, do you know when the version 1.4.0 of this librabry will be released on gulp?

Thanks

[scotttrinh]

@brunodeprez Thanks for reaching out! Sorry I'm a bit late working on the 1.4.0 release. If you want to use @hozkok 's branch with npm, you can use it by referencing his fork in your package.json while you're waiting for this feature to land (hopefully before the end of the year).

[brunodeprez]

Thanks @scotttrinh , I must say I did not know I could do that ... thanks for the hint!!

[scotttrinh]

@hozkok

I'm looking at little harder at this PR, and it seems that the way this is implemented might be a breaking change. If you have existing an existing instance that doesn't have the ${name}#${storeName} naming convention, is there a way to retrieve it? For instance, in my own app, I never do any configuration so my instance is named lf. How I read this code, it would look for lf#keyvaluepairs. Just tried it on my own project and I'm getting "No localForage instance of that name exists." with either instance('lf') or instance({ name: 'lf', storeName: 'keyvaluepairs' }).

[hozkok]

Hi @scotttrinh,

You are right, it might be a breaking change for existing users.

One way to handle this situation might be to deprecate the use of instance(name) where name is a string. And if people use it with string, we can return lfInstances[name] rather than lfInstances[name + '#' + storeName] in this line and this line.

And if the use case is the new object format; e.g. instance({name}) || instance({name, storeName}) || instance({storeName}), we can return the newly used ${name}#${storeName} format so it wouldn't break anything.

I have already updated the old usage as DEPRECATED in README.md file.

If you think this is a good idea, I can update these two lines with a commit.

Thanks for detecting this issue!

[scotttrinh]

@hozkok

That seems like a nice backwards compatible change since existing users would definitely be using instance with a string since that was the old type. If you can update that and write a test case around it, I'll get it merged ASAP and do a release. Thanks again for the effort you've already put into this!

[hozkok]

@scotttrinh Thinking a bit further on it, for anyone to reach a database with instance function, they have to createInstace first. so there wouldn't be anything broken. In your use case also, you called instance function without creating it. It is not allowed in the previous version too.

Being said that, I will still update this line to let instance() function with no argument to return global (default) localforage instance.

[scotttrinh]

Does this solve for the case where a user made a localForage instance on an old version? In my project, I'm not ever using instance or createInstance.

[hozkok]

For the existing users, accessing $localForage without instance or createInstance will still refer to lfInstances[name], so there is no problem with it. I also changed $localForage.instance() (with no arguments) to refer to lfInstances[name] as well. So there isn't anything broken. the only thing that uses lfInstances[name] is the global localforage itself ($localForage also refers to it in this case).

However, I just realized that bind and unbind functions are also using lfInstances[name] and definitely going to be a broken. (and these functions are the only functions left which use lfInstances[name])

I was surprised how these functions passed the test, but I see that bind and unbind functions' tests are not complete. Will also update them.

I am going to update these functions and their tests and push a commit as soon as possible.

[scotttrinh]

🎉 @hozkok for MVP! 🎉

Thanks for the hard work!

[roelofjan-elsinga]

I'd love to see this implemented, because I'm trying to share an instance between two applications on the same URL. It looks like a great new addition.

[scotttrinh]

@hozkok any update on the work you were doing to get this going? I'm happy to take it over since it seems like a really good feature that people want if you're not actively interested in finishing here, no sweat.

[MusicCog]

@scotttrinh Please take this patch up.
Looks like @hozkok solved the issue then has not come back.

[scotttrinh]

@MusicCog Alright, I'll stick it on my TODO list. Should be later this week that I have a patch. For those who are looking to use this feature, what problem are we trying to solve? I've read the code a few times and I see what it does, but I'm still unclear. There is basically a registry of names in an object, and this just allows you to pseudo-namespace by making the key name + '#' + storeName which seems like you'd be able to accomplish that just by using a unique name in the first place. Just want to make sure I can set up some test cases and know they're the actual use case people are looking for.

[brunodeprez]

@scotttrinh if I use the name + '#' + storeName (for instance lf#feature) with your version, I get a new database called 'lf#feature', which contains on store called keyvaluepairs.

If I use @hozkok version, I get one database called 'lf', and inside this database one store called 'feature', which is what I am looking for as I will have other stores dynamically added by my app during run time. I hope this helps and emphase how important this PR is for me.

Cheers and thanks for all your great work

[scotttrinh]

@brunodeprez Thanks for the clarification. I'll take a look at this today and see if we can push this thing over the finish line.

[scotttrinh]

Just wanted to drop a quick update here on my progress:

As alluded to, the test coverage is a bit lacking, and I'm trying to be careful not to introduce any breaking changes (again! 🙀). Reviewing the tests more closely, I realized that if I would have written this from scratch in the production project I use it on, I would have a comprehensive unit test and integration test suite, so I really want to get us up to that level, where we have a fair amount of confidence that we're not breaking backwards compatibility.

To that end, I'm almost done writing the unit tests–just bind/unbind/prefix to go. Once those are done, I plan on writing some integration tests and fixing the issues with the CI so we can be sure IE8+/Modern work with the code.

Once that's 100% complete, I'll readdress this so that it's 100% backwards compatible, and it should be a piece of cake to get it merged.

Thanks for y'all's patience!