slact / nchan.js

NPM package for the Javasript client for Nchan
Other
91 stars 25 forks source link

Cannot Run in Browser Worker #7

Closed The-Don-Himself closed 6 years ago

The-Don-Himself commented 7 years ago

The hard-coded dependency on the window object throws errors when running this library in a browser worker where the Window is not defined. Specifically, line 81

"use strict"; 
function NchanSubscriber(url, opt) {
  if(this === window) {
    throw "use 'new NchanSubscriber(...)' to initialize";
  }

I think a simple fix would be to check whether window exists first

if (typeof window !== 'undefined' && this === window)
slact commented 7 years ago

You're quite right. You're welcome to submit a pull request if you want.

The-Don-Himself commented 7 years ago

Thanks @slact , in fact I do and would have just submitted one instead of creating this issue but I thought perhaps there might be a few other places you might have in mind where you'd want to add the same checks. For those who want to use this lib in a worker, the above check works. Thought?

slact commented 7 years ago

what about this :

      if(typeof window == "object") {
        loc = window.location;
      }

https://github.com/slact/nchan.js/blob/master/NchanSubscriber.js#L587-L589

I know that won't cause issues, but does a worker have a useful location property? I use this code to allow for relative websocket urls.

The-Don-Himself commented 7 years ago

Yes browsers have read-only access to the location property

https://www.html5rocks.com/en/tutorials/workers/basics/#toc-enviornment

I agree that check doesn't seem like it will cause issues. I think I remember seeing a similar one, https://github.com/slact/nchan.js/blob/master/NchanSubscriber.js#L57

The-Don-Himself commented 7 years ago

I would like us to close this issue (I like to keep a very low issues count :), Should I just add a couple of typeof checks before the window object and submit a PR?

slact commented 7 years ago

yep, go ahead

The-Don-Himself commented 6 years ago

Yeah, sorry I forgot to close this one, thanks for merging though :+1: