Skip to content
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

use localforage in a serviceworker #402

Closed
remkoboschker opened this issue Jul 15, 2015 · 30 comments
Closed

use localforage in a serviceworker #402

remkoboschker opened this issue Jul 15, 2015 · 30 comments

Comments

@remkoboschker
Copy link
Contributor

Hi,

I would like to use localforage in a serviceworker. Version 1.2.4 in Chrome 43 throws a reference error on line 963 "window undefined", because there is no window in the serviceworker.

Window was removed in #144 but reintroduced in #237.

Should localforage or should it not be usable in a webworker / serviceworker environment?

I'll see if I can get it to work.

@tofumatt
Copy link
Member

It is a design goal of localforage to work in web workers. You're right though ; it currently doesn't work.

If you can send a pull request that fixes it that would be great, but if not know it's something I'll eventually work on :-)

  • tofumatt

On Wed, Jul 15, 2015 at 10:06 AM, Remko Boschker [email protected]
wrote:

Hi,
I would like to use localforage in a serviceworker. Version 1.2.4 in Chrome 43 throws a reference error on line 963 "window undefined", because there is no window in the serviceworker.
Window was removed in #144 but reintroduced in #237.
Should localforage or should it not be usable in a webworker / serviceworker environment?

I'll see if I can get it to work.

Reply to this email directly or view it on GitHub:
#402

@remkoboschker
Copy link
Contributor Author

replacing .call(window) by .call(this) loads the lib just fine, but this probably introduces other issues. I'll fork and run some tests later today. Thanks.

@tofumatt
Copy link
Member

Awesome, thanks so much.

If you run the tests using npm test you can run them in phantomjs, and using grunt serve should allow you to run them from browsers using a local node server URL.

  • tofumatt

On Wed, Jul 15, 2015 at 10:27 AM, Remko Boschker [email protected]
wrote:

replacing .call(window) by .call(this) loads the lib just fine, but this probably introduces other issues. I'll fork and run some tests later today. Thanks.

Reply to this email directly or view it on GitHub:
#402 (comment)

@thgreasi
Copy link
Member

.call(typeof window !== 'undefined' ? window : this)
Looks like a safe bet :-)

On Wed, Jul 15, 2015, 12:31 Matthew Riley MacPherson <
[email protected]> wrote:

Awesome, thanks so much.

If you run the tests using npm test you can run them in phantomjs, and
using grunt serve should allow you to run them from browsers using a
local node server URL.

  • tofumatt

On Wed, Jul 15, 2015 at 10:27 AM, Remko Boschker <[email protected]

wrote:

replacing .call(window) by .call(this) loads the lib just fine, but this
probably introduces other issues. I'll fork and run some tests later today.

Thanks.

Reply to this email directly or view it on GitHub:
#402 (comment)


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

@remkoboschker
Copy link
Contributor Author

😄 sure, will use it.

@thgreasi
Copy link
Member

This might be useful.

@thgreasi
Copy link
Member

Just took a shot on this, the web workers part, at least... Hope it will
work with service workers similarly.
Since we now deal with es6 modules, we are in "use strict" mode by default
(and babel honors this semantic). As a result this is undefined inside
IIFEs...

On Wed, Jul 15, 2015, 14:57 Remko Boschker [email protected] wrote:

[image: 😄] sure, will use it.


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

@remkoboschker
Copy link
Contributor Author

In the service worker your fix raises

Uncaught (in promise) TypeError: self._initStorage is not a function at http://localhost:8080/js/lib/localforage.js:265:48

.call(typeof window !== 'undefined' ? window : this) does not raise this exception

Also a unit test fails

  1) Config API Driver API "before each" hook:
     ReferenceError: Can't find variable: localforage
      at http://localhost:9999/test/test.drivers.js:11
      at http://localhost:9999/bower_components/mocha/mocha.js:4315
      at next (http://localhost:9999/bower_components/mocha/mocha.js:4626)
      at http://localhost:9999/bower_components/mocha/mocha.js:4630
      at timeslice (http://localhost:9999/bower_components/mocha/mocha.js:5761)

I'll work on a fix

@thgreasi
Copy link
Member

We might just have to use a different var name for the scope. Maybe service
workers do not like redefining self.

On Mon, Jul 20, 2015, 11:37 Remko Boschker [email protected] wrote:

In the service worker your fix raises

Uncaught (in promise) TypeError: self._initStorage is not a function at http://localhost:8080/js/lib/localforage.js:265:48

Also a unit test fails

  1. Config API Driver API "before each" hook:
    ReferenceError: Can't find variable: localforage
    at http://localhost:9999/test/test.drivers.js:11
    at http://localhost:9999/bower_components/mocha/mocha.js:4315
    at next (http://localhost:9999/bower_components/mocha/mocha.js:4626)
    at http://localhost:9999/bower_components/mocha/mocha.js:4630
    at timeslice (http://localhost:9999/bower_components/mocha/mocha.js:5761)

I'll work on a fix


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

@thgreasi
Copy link
Member

Can you share your service worker test code?

On Mon, Jul 20, 2015, 13:29 Thodoris Greasidis [email protected] wrote:

We might just have to use a different var name for the scope. Maybe
service workers do not like redefining self.

On Mon, Jul 20, 2015, 11:37 Remko Boschker [email protected]
wrote:

In the service worker your fix raises

Uncaught (in promise) TypeError: self._initStorage is not a function at http://localhost:8080/js/lib/localforage.js:265:48

Also a unit test fails

  1. Config API Driver API "before each" hook:
    ReferenceError: Can't find variable: localforage
    at http://localhost:9999/test/test.drivers.js:11
    at http://localhost:9999/bower_components/mocha/mocha.js:4315
    at next (http://localhost:9999/bower_components/mocha/mocha.js:4626)
    at http://localhost:9999/bower_components/mocha/mocha.js:4630
    at timeslice (http://localhost:9999/bower_components/mocha/mocha.js:5761)

I'll work on a fix


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

@remkoboschker
Copy link
Contributor Author

Hi,

I mistakenly took the src instead of the build with the drivers in the file. Sorry for that.

I noticed that window has a property self that returns itself. Perhaps just replacing this with self will do. I'll add a unit test for service-workers tomorrow.

@thgreasi
Copy link
Member

Yes, I had that in mind, but wanted to avoid possible problems with the
different module bundlers/loaders.
self is one of the most common variable names (along with that) used
for sharing context with closures (it's used by localforage as well, as you
noticed). As a result, there are cases that people overwrite it without
even knowing its existence.
Anyway if have anything against my approach, I would be happy to change
that with a new commit.

On Tue, Jul 21, 2015, 17:25 Remko Boschker [email protected] wrote:

Hi,

I mistakenly took the src instead of the build with the drivers in the
file. Sorry for that.

I noticed that window has a property self that returns itself. Perhaps
just replacing this with self will do. I'll add a unit test for
service-workers tomorrow.


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

@thgreasi
Copy link
Member

Let me rephrase that, since I think I might missed my point/wasn't clear:

  • For most devs/libs, window is the global object in JS. So for everyday webdev window is respencted and not redeclared. Some of them ignore the existence of self and even more redeclare it to pass contexts around.
  • For *worker devs/libs, self is the global object by default. We can bet that they use it carefully, since otherwise bugs should have appear.

Still, I have no problem using just self if you think that all the above is just an overkill.

@remkoboschker
Copy link
Contributor Author

@thgreasi I do not mind the extra precaution. Can you confirm that the test are passing in your localforage#webworkers-fix? I tried to copy the webworkers test to a serviceworker scenario but the tests are not passing. When I include only the test.webworker.js in the test.component.html not a single test is run.

@tofumatt
Copy link
Member

I believe the webworker tests are commented out right now, or more accurately are marked as skip. You might need to change them to fix this.

Let me know if it seems like that's the case but you need more help.


Sent from Mailbox

On Thu, Jul 23, 2015 at 10:12 AM, Remko Boschker [email protected]
wrote:

@thgreasi I do not mind the extra precaution. Can you confirm that the test are passing in your localforage#webworkers-fix? I tried to copy the webworkers test to a serviceworker scenario but the tests are not passing. When I include only the test.webworker.js in the test.component.html not a single test is run.

Reply to this email directly or view it on GitHub:
#402 (comment)

@thgreasi
Copy link
Member

@tofumatt, @remkoboschker
I removed the skip in PR #406 and seems to work with changes I did to call(...) and the babel system import plugin.
I can verify that #406 fixes web-workers and passes the test cases both on Travis and locally/in-browser.

@remkoboschker I also started looking at a service worker test case (today morning), but it is still in embryonic state I think.

@remkoboschker
Copy link
Contributor Author

@thgreasi I have a basic unit test
serviceworker-client.js:

importScripts("/dist/localforage.js");

self.oninstall = function(event) {
   event.waitUntil(
       localforage
       .setItem('serviceworker', 'serviceworker present')
       .then(function(value) {
           console.log(value);
       })
   );
};

test.serviceworker.js:

describe('serviceworkers', function() {
    it('should set a value', function(done) {
       navigator
       .serviceWorker
       .register('/test/serviceworker-client.js')
       .then(function() {
           return localforage.getItem('serviceworker');
       })
       .then(function(result) {
           expect(result).to.equal('serviceworker present');
           done();
       })
       .catch(function(error) {
           done(error);
       });
   });
});

It runs in a browser, but not in Phantomjs; don't know why

@thgreasi
Copy link
Member

That looks very similar to my code! On top of that, I'm currently trying to
post the desired driver name to the service worker.

On Thu, Jul 23, 2015, 14:57 Remko Boschker [email protected] wrote:

@thgreasi https://github.com/thgreasi I have a basic unit test
serviceworker-client.js:

importScripts("/dist/localforage.js");

self.oninstall = function(event) {
event.waitUntil(
localforage
.setItem('serviceworker', 'serviceworker present')
.then(function(value) {
console.log(value);
})
);
};

test.serviceworker.js:

describe('serviceworkers', function() {
it('should set a value', function(done) {
navigator
.serviceWorker
.register('/test/serviceworker-client.js')
.then(function() {
return localforage.getItem('serviceworker');
})
.then(function(result) {
expect(result).to.equal('serviceworker present');
done();
})
.catch(function(error) {
done(error);
});
});
});


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

@thgreasi
Copy link
Member

Can I find this code in any public commit that I can cherry pick and push
on top of my PR? (And also possibly cherry pick it retaining your commit
info.)

On Thu, Jul 23, 2015, 16:55 Thodoris Greasidis [email protected] wrote:

That looks very similar to my code! On top of that, I'm currently trying
to post the desired driver name to the service worker.

On Thu, Jul 23, 2015, 14:57 Remko Boschker [email protected]
wrote:

@thgreasi https://github.com/thgreasi I have a basic unit test
serviceworker-client.js:

importScripts("/dist/localforage.js");

self.oninstall = function(event) {
event.waitUntil(
localforage
.setItem('serviceworker', 'serviceworker present')
.then(function(value) {
console.log(value);
})
);
};

test.serviceworker.js:

describe('serviceworkers', function() {
it('should set a value', function(done) {
navigator
.serviceWorker
.register('/test/serviceworker-client.js')
.then(function() {
return localforage.getItem('serviceworker');
})
.then(function(result) {
expect(result).to.equal('serviceworker present');
done();
})
.catch(function(error) {
done(error);
});
});
});


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

@remkoboschker
Copy link
Contributor Author

I have pushed the branch to https://github.com/curit/localForage. In test/test.serviceworker.js en in test/serviceworker-client.js an earlier attempt to send messages is commented out. I found this example to be quite useful https://jakearchibald.github.io/isserviceworkerready/demos/postMessage/

Good luck!

@thgreasi
Copy link
Member

Is there any chance you can rebase over the latest commit of my PR (and
even better squash some commits)? I'm getting a huge diff on the dist
files even though I was just expecting the extra test case.
On the other hand I don't want to spam you, the service worker test case
could also be a separate PR :)

On Thu, Jul 23, 2015, 17:49 Remko Boschker [email protected] wrote:

I have pushed the branch to https://github.com/curit/localForage. In
test/test.serviceworker.js en in test/serviceworker-client.js an earlier
attempt to send messages is commented out. I found this example to be quite
useful
https://jakearchibald.github.io/isserviceworkerready/demos/postMessage/

Good luck!


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

@remkoboschker
Copy link
Contributor Author

@thgreasi sorry about the diff I was trying to get the unit test to run at all. I'm working on it. By the way how do I get at the pull request in git or should I fork your repo?

@remkoboschker
Copy link
Contributor Author

Turns out the component-build was not being executed. So now I can run the tests and have submitted a pull request to your repo. I will now try to get the tests to pass.

I found this useful to fetch the pull request https://help.github.com/articles/checking-out-pull-requests-locally/

@remkoboschker
Copy link
Contributor Author

thgreasi#1 now has both tests passing when run in Chrome 44

@remkoboschker
Copy link
Contributor Author

and in firefox nightly with the dom.serviceworker enabled

@remkoboschker
Copy link
Contributor Author

It does not look like phantomjs supports serviceworkers. I have asked some questions jakearchibald/isserviceworkerready#43 and ariya/phantomjs#13437

@thgreasi
Copy link
Member

Just 1 thing to note: awesome 👍

@remkoboschker
Copy link
Contributor Author

@thgreasi Thank you. I'm away on holiday the coming weeks. So good luck with the pull request!

@thgreasi
Copy link
Member

@remkoboschker best wishes regarding your holidays. I will try to finish this up by the time you are back.
Thanks for your collaboration.

@thgreasi
Copy link
Member

@remkoboschker Awesome work!!! I'm speechless 😮 😮 😮 😃
I will try to merge it today or at most tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants