eclipse / paho.mqtt.javascript

paho.mqtt.javascript
Other
1.15k stars 467 forks source link

Brings Paho one step closer to be supported in non-browser environments. #153

Closed manueliglesias closed 6 years ago

manueliglesias commented 6 years ago

This brings Paho one step closer to be supported in non-browser environments.

Fixes: #147

manueliglesias commented 6 years ago

@jpwsutton would you consider something like this too?

https://github.com/eclipse/paho.mqtt.javascript/compare/eclipse:285bbdd...manueliglesias:f5cefde

jpwsutton commented 6 years ago

Thanks for this contribution @manueliglesias, getting closer to non browser environments is a big priority for us, so this is really welcome!

This change (https://github.com/eclipse/paho.mqtt.javascript/compare/eclipse:285bbdd...manueliglesias:f5cefde) looks like it could be a good fallback for cases where localStorage isn't available, how would you envisage non-browser environments using their own localStorage API?

manueliglesias commented 6 years ago

Hey @jpwsutton

I imagine that in the case you really want to use localStorage but is not available in your environment, you could simply set the global yourself to an implementation of your liking, e.g. using node-localstorage:

if (typeof localStorage === "undefined" || localStorage === null) {
  var LocalStorage = require('node-localstorage').LocalStorage;
  global.localStorage = new LocalStorage('./scratch');
}

One of the things I personally like about paho-mqtt is that it doesn't depend on more modules, but of course another option would be to abstract the storage piece and use something like localForage

Let me know what path you envision for Paho regarding this and I'd love to contribute. 😃

undefobj commented 6 years ago

@jpwsutton do you have an ETA on when we might get a merge for this? We work for AWS with several packages using this Paho client and many customers experiencing an issue with Server Side Rendering that this PR resolves. Happy to chat over a call through this as well if it would help expedite things. Thanks!

jpwsutton commented 6 years ago

@manueliglesias I like your solution, it keeps everything clean but allows for people to override things when they need to. Could you add that into this merge, then I'll merge it all in together, I've run all of the tests against it and can't see any backwards compatibility issues, so I'm happy! 😄

jpwsutton commented 6 years ago

@undefobj I think we've had enough changes to warrants adding this client into the Eclipse Photon release that will be happening in June (https://projects.eclipse.org/projects/iot.paho/releases/1.4.0-photon/review). I've created Issue #155 to track this.

manueliglesias commented 6 years ago

Thank you @jpwsutton , I've included the localStorage fallback into this PR

undefobj commented 6 years ago

@jpwsutton thanks very much, if there is anything we can do to help review/streamline this further with more documentation please let myself or @manueliglesias know. There's high customer demand for SSR and just want to keep the people pinging us up to date. We appreciate your looking at our contributions!

manueliglesias commented 6 years ago

@jpwsutton also I noticed that the next release/publish would be at the end of June. Is there any way we might be able to move the merging and npm publish of this PR up sooner? Alternatively, would it make sense for us to distribute a modified version of this library? We would rather not do that but there are a lot of customers asking us for this daily as they're unable to meet their business needs around SSR without this PR.

growak commented 6 years ago

@manueliglesias, thank for your work on the server side rendering! Do you have any quick and dirty hack to let anybody work with appsync with nextjs or nuxt ? I tried to use polyfill without any success at this time ! Thx