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.

Sign up for GitHub

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

Jump to bottom

Version 4.2.0: ws does not work in the browser #1141

Closed
ttsirkia opened this issue Aug 13, 2020 · 24 comments
Closed

Version 4.2.0: ws does not work in the browser #1141

ttsirkia opened this issue Aug 13, 2020 · 24 comments
Assignees
@YoDaMa
Labels
bug

Comments

@ttsirkia
Copy link

After the latest update, my web page using mqtt and MapBox GL JS library fails to fetch resources and gives this error:

Error: ws does not work in the browser. Browser clients must use the native WebSocket object

The error comes from inside mapbox-gl-js library. Does the new version somehow make global changes to some prototypes etc. which would cause this? The error disappeared after downgrading back to 4.1.0.
@ttsirkia
Copy link
Author

Both libraries are attached with script tags to the page.

@YoDaMa
Copy link
Contributor

YoDaMa commented Aug 13, 2020 via email

@ttsirkia
Copy link
Author

ttsirkia commented Aug 13, 2020 •
edited
Loading

@YoDaMa I'll try to provide a simple example. The problem is with the MapBox authentication token to make it work.

@ttsirkia ttsirkia changed the title Version 4.2.0 doesn't work with mapbox-gl-js Version 4.2.0 doesn't work from unpkg Aug 13, 2020
@ttsirkia
Copy link
Author

OK, mapbox-gl-js is not related here. It seems that the unpkg version causes this.

<!doctype html>
<html>
  <head>
    <meta charset="utf-8">
    <script src="https://unpkg.com/mqtt/dist/mqtt.min.js"></script>
    <script>
      var client = mqtt.connect('wss://mqtt.hsl.fi:443');
      client.on('connect', function() {});
    </script>
  </head>
  <body>



  </body>
</html>

This will cause the error.

@lucaong
Copy link

lucaong commented Aug 13, 2020 •
edited
Loading

Observing the same issue in a browser app built with Webpack, so it does not seem to be related to unpkg. Since the 4.2.0 update, the app breaks with:

Error: ws does not work in the browser. Browser clients must use the native WebSocket object

Downgrading to 4.1.0 fixes it. Is ws used also for when using MQTT in a browser environment?

@lucaong
Copy link

lucaong commented Aug 13, 2020

@YoDaMa It looks like MQTT now uses ws instead of websocket-stream:

MQTT.js/lib/connect/ws.js

Line 3 in b2c1215

var WebSocket = require('ws')

The problem is that ws only works in Node, not in the browser, differently from websocket-stream. Therefore, 4.2.0 breaks browser builds.

One way to fix this would be to replace ws with the native WebSocket in browser builds, like websocket-stream does.

@YoDaMa
Copy link
Contributor

YoDaMa commented Aug 13, 2020 via email

@YoDaMa YoDaMa self-assigned this Aug 13, 2020
@YoDaMa YoDaMa added the bug label Aug 13, 2020
@YoDaMa
Copy link
Contributor

YoDaMa commented Aug 13, 2020

yeah so like you said we need to implement an isomorphism for the browser compatibility to work with the Websocket in browser builds. I'm working on a PR rn to fix this.

@ttsirkia ttsirkia mentioned this issue Aug 14, 2020
Closed
@ttsirkia ttsirkia changed the title Version 4.2.0 doesn't work from unpkg Version 4.2.0: ws does not work in the browser Aug 14, 2020
@seriousme
Copy link
Contributor

FYI:
Looking at the docs for ws the isomorphic fix seems to be easy:

Note: This module does not work in the browser. The client in the docs is a reference to a back end with the role of a
client in the WebSocket communication. Browser clients must use the native WebSocket object. To make the same 
code work seamlessly on Node.js and the browser, you can use one of the many wrappers available on npm, 
like isomorphic-ws.

and isomorphic-ws usage seems to be simple as well: https://www.npmjs.com/package/isomorphic-ws

@ysfscream
Copy link
Contributor

ysfscream commented Aug 18, 2020 •
edited
Loading

Yes, the ws module does not work in the browser.

image

@YoDaMa
Copy link
Contributor

YoDaMa commented Aug 18, 2020 via email

@pr0x1
Copy link

pr0x1 commented Aug 18, 2020

Hi @YoDaMa
Thanks for you answer to solve this issue.

when the update to solve the bug will be release?

Reggards.

@YoDaMa
Copy link
Contributor

YoDaMa commented Aug 19, 2020

Can someone in this thread try using the patch provided in #1145 . I've tested it manually on my machine and it seems to be working but I'd like other folks trying it out. @pr0x1 @ysfscream @lucaong

@lucaong
Copy link

lucaong commented Aug 19, 2020

Thanks, I’ll try it tomorrow and let you know.

@pr0x1
Copy link

pr0x1 commented Aug 20, 2020 •
edited
Loading

Hi @YoDaMa I was testing but the issue continues.

mqtt js

i'm using the following script

<script src="https://unpkg.com/mqtt/dist/mqtt.min.js"></script>

Regards.

@YoDaMa
Copy link
Contributor

YoDaMa commented Aug 20, 2020 via email

@ysfscream
Copy link
Contributor

@YoDaMa 👍 Thanks for the fix. I will test it later.

@nosovk
Copy link
Contributor

nosovk commented Aug 20, 2020 •
edited
Loading

really say there is lots of problems with running mqtt in browser. Not only ws, but readable-stream fails to run in browser if node shims are not provided.
nodejs/readable-stream#412 that lead to goal of supporting shim free version in next release nodejs/readable-stream#439 (because webpack 5 will drop node shim support https://webpack.js.org/migrate/5/#test-webpack-5-compatibility, and webpack is most common builder for now)

even if we removed methods that require shiming from mqtt (#1134) we still can't use mqtt.js in browser without hacks because of underlying libs. As a result we need to maintain hacks like that https://github.com/sclausen/ngx-mqtt/pull/158/files

really say I don't see how to improve situation, except providing node shims via webpack in each dependant project. Problem with WS is just one of the symptoms.

@lucaong
Copy link

lucaong commented Aug 20, 2020

@YoDaMa , I just tried out your patch in #1145 and it works: no error, and MQTT connects successfully while running in the browser. 👍

@lucaong
Copy link

lucaong commented Aug 20, 2020

@nosovk I am not sure what you mean, I have been using MQTT.js in the browser for years now, connecting via Websocket, and never experienced any problem. Maybe I miss some context, but I really think that supporting browsers is a very important goal for MQTT.js, as that's one of the primary environments where JS runs.

@YoDaMa
Copy link
Contributor

YoDaMa commented Aug 20, 2020

yes sorry @nosovk I'm a bit confused at what you're getting at

@mbradley612 mbradley612 mentioned this issue Aug 22, 2020
Closed
@AlxZchk AlxZchk mentioned this issue Aug 22, 2020
Closed
5 tasks
@YoDaMa
Copy link
Contributor

YoDaMa commented Aug 24, 2020

release 4.2.1 (#1149) fixes this.

@YoDaMa YoDaMa closed this as completed Aug 24, 2020
@lucaong
Copy link

lucaong commented Aug 25, 2020

Thanks @YoDaMa for fixing this :)

@nosovk
Copy link
Contributor

nosovk commented Sep 2, 2020

@YoDaMa @lucaong sorry for late response, I'le provide some explanation.

Current browser version of MQTT.js is build via browserify. Browserify is predecessor of webpack, it's a bundler. What does it mean? When you make browser build of mqtt.js you ship mqtt bundle combined with all deps. For example that one https://github.com/mqttjs/MQTT.js/blob/master/package.json#L76 and other.

It's ok, if you add mqtt js like external module in yours application. That bundle is minified and has no external requirements.

But some time after Browserify there was Webpack/Rollup was invented. They works in a bit different.way. For webpack good library is not the one which provides compiled\minified js file. For webpack\rollup good dependency is js code, which is not minified and surely not bundled with deps. Rollup will try to treeshacke librarry deps with other packages deps, and split to smaller and probably even lazy loaded chunks. And splitting/minifying should be done according to the project config (not library config).

Good example is Angular. Angular requires libraries to provide es, umd, esm etc bundles. Package spec: https://docs.google.com/document/d/1CZC2rcpxffTDfRDs6p1cfbmKNLA6x5O-NtkJglDaBVs/preview

Alternate solution - allow Angular to build all needed versions itself. But it's only possible only if source code of the library is ready to be complied by webpack. In case of mqtt it means that nodejs specific methods shouldn't be invoked. For example process etc. Currently mqtt code could be builded with webpack\rollup, but one of dependant libs - is not.

webpack\rollup couldn't process nodejs/readable-stream.
nodejs/readable-stream#426
nodejs/readable-stream#412

It will be fixed, but currently all libs that depend on readable-stream have to use browserified (non tree shakable) version or enable node shims (polyfils). For webpack versions prior to 5 it wasn't a big problem because of default webpack configuration have those polyfils enabled. But for Rollup or Angular projects polyfills should be explicitly defined. For example check sclausen/ngx-mqtt@b488b80
It's surely not a good solution
.
Probably after nodejs/readable-stream#439 issues with node intrnals used in browser will disappear, and then we will be able to use mqtt.js in Angular project without hacks.

Using mqtt with webpack now could be a bit tricky (with webpack 5 probably not possible)

@Juraj-Masiar Juraj-Masiar mentioned this issue Apr 22, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@YoDaMa YoDaMa

Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@lucaong @nosovk @ttsirkia @seriousme @YoDaMa @ysfscream @pr0x1