chore: replace `browserify` with `rollup` #1632

robertsLando · 2023-07-07T09:33:53Z

No description provided.

codecov · 2023-07-07T09:38:05Z

Codecov Report

Patch coverage: 81.11% and project coverage change: -4.64% ⚠️

Comparison is base (d71b000) 85.74% compared to head (fa6ab27) 81.11%.
Report is 24 commits behind head on main.

❗ Current head fa6ab27 differs from pull request most recent head ed09bf9. Consider uploading reports for the commit ed09bf9 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1632      +/-   ##
==========================================
- Coverage   85.74%   81.11%   -4.64%     
==========================================
  Files          13       21       +8     
  Lines        1249     1366     +117     
  Branches        0      324     +324     
==========================================
+ Hits         1071     1108      +37     
- Misses        178      181       +3     
- Partials        0       77      +77

Files Changed	Coverage Δ
src/lib/handlers/auth.ts	`20.00% <20.00%> (ø)`
src/lib/connect/ws.ts	`36.80% <36.80%> (ø)`
src/lib/is-browser.ts	`75.00% <75.00%> (ø)`
src/lib/handlers/connack.ts	`77.41% <77.41%> (ø)`
src/lib/handlers/index.ts	`81.57% <81.57%> (ø)`
src/lib/connect/index.ts	`81.63% <81.63%> (ø)`
src/lib/topic-alias-send.ts	`83.87% <83.87%> (ø)`
src/lib/shared.ts	`84.21% <84.21%> (ø)`
src/lib/client.ts	`85.81% <85.81%> (ø)`
src/lib/connect/tls.ts	`86.95% <86.95%> (ø)`
... and 11 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…browserify

robertsLando · 2023-07-12T07:13:09Z

@vishnureddy17 @BertKleewein I made some changes here but I still get this error when i run the browser test:

TypeError: (intermediate value).parser is not a function\n

Seems that rollup output boundle makes some strange tweaks on mqtt-packet. In that line this.constructor is null and this points to EventEmitter, not Parser instance.

Couldn't find anything to fix this issue, I just found a possible reference to it here: vitejs/vite#9703 but it doesn't solve the issue

robertsLando · 2023-07-12T07:20:21Z

Consider also replacing terser with a rollup pluigin: https://rollupjs.org/configuration-options/#output-plugins

vishnureddy17 · 2023-07-12T15:06:33Z

Unfortunately, my knowledge on browser JS and and bundling is very limited :/

BertKleewein · 2023-07-12T21:53:13Z

The EventEmitter problem happens because require('events') returns something that is both a constructor and a module.

In other words, callers can choose to do either const EventEmitter = require('events') or they can do const EventEmitter = require('events').EventEmitter. In node.js, these 2 methods are interchangeable. In the browser, they're apparently not interchangeable.

One fix is to update the mqtt-packet module to use require('events').EventEmitter, assuming that this new syntax works on all not-ancient versions of node.js.

If you make this change to mqtt-packet, the next problem that pops up is where readable-stream uses process.nextTick and process is undefined. According to my rollup config, this should have been polyfilled, but that's not happening for me. I don't know why.

robertsLando · 2023-07-13T06:32:17Z

Thanks for the explanation @BertKleewein !

Anyway I also opened an issue on rollup yesterday and turns out that's a bug with rollup commonjs plugin... sad story this is my first time using rollup and I also found a bug, good news is that the user that answered my issue also created this PR to fix it. Hope it will not take too much time.

I will also try to see if I can fix the readable-stream issue

Some references to readable-stream error:

rollup/rollup-plugin-commonjs#293
rollup/rollup#1507

robertsLando · 2023-07-13T08:08:52Z

Ok some steps forward. I used @BertKleewein suggestion to fix the mqtt-packet parser issue, I firstly tried by using rollup replace plugin as I did later for readable-stream but without success so I manually patched the file. So now tests are starting but the output stucks on client.end:

/MQTT.js$ npm run unit-test:browser

> [email protected] unit-test:browser
> airtap --server test/browser/server.js test/browser/test.js

tunnelled server started on port 44229
TAP version 13
# MQTT.js browser test
client connect
ok 1 no error on subscribe
ok 2 no error on publish
ok 3 should match topic
ok 4 should match payload

Not sure if that's related to some compatibility issues with the steam module :(

robertsLando · 2023-07-13T08:13:59Z

With debug enabled:

npm run unit-test:browser

> [email protected] unit-test:browser
> airtap --server test/browser/server.js test/browser/test.js

tunnelled server started on port 42099
MqttClient :: options.protocol ws
MqttClient :: options.protocolVersion 3
MqttClient :: options.username null
MqttClient :: options.keepalive 60
MqttClient :: options.reconnectPeriod 1000
MqttClient :: options.rejectUnauthorized null
MqttClient :: options.properties.topicAliasMaximum null
MqttClient :: clientId mqttjs_857bf1ef
MqttClient :: setting up stream
_setupStream :: calling method to clear reconnect
_clearReconnect : clearing reconnect timer
_setupStream :: using streamBuilder provided to client to create stream
_setupStream :: pipe stream to writable stream
_setupStream: sending packet `connect`
sendPacket :: packet: { cmd: 'connect' }
sendPacket :: emitting `packetsend`
sendPacket :: writing to stream
sendPacket :: writeToStream result true
TAP version 13
# MQTT.js browser test
writable stream :: parsing buffer
work :: getting next packet in queue
work :: no packets in queue
work :: done flag is true
writable stream :: parsing buffer
work :: getting next packet in queue
work :: no packets in queue
work :: done flag is true
writable stream :: parsing buffer
work :: getting next packet in queue
work :: no packets in queue
work :: done flag is true
writable stream :: parsing buffer
parser :: on packet push to packets array.
work :: getting next packet in queue
work :: packet pulled from queue
_handlePacket :: emitting packetreceive
_handleConnack
_setupPingTimer :: keepalive 60 (seconds)
connect :: sending queued packets
deliver :: entry null
_resubscribe
client connect
subscribe: array topic hello
subscribe: pushing topic `hello` and qos `0` to subs list
subscribe :: resubscribe true
subscribe :: call _sendPacket
_sendPacket :: (mqttjs_857bf1ef) ::  start
sendPacket :: packet: {
  cmd: 'subscribe',
  subscriptions: [ { topic: 'hello', qos: 0 } ],
  qos: 1,
  retain: false,
  dup: false,
  messageId: 50292
}
sendPacket :: emitting `packetsend`
sendPacket :: writing to stream
sendPacket :: writeToStream result true
sendPacket :: invoking cb
nop :: null
writable stream :: parsing buffer
work :: getting next packet in queue
work :: no packets in queue
work :: done flag is true
writable stream :: parsing buffer
work :: getting next packet in queue
work :: no packets in queue
work :: done flag is true
writable stream :: parsing buffer
work :: getting next packet in queue
work :: no packets in queue
work :: done flag is true
writable stream :: parsing buffer
parser :: on packet push to packets array.
work :: getting next packet in queue
work :: packet pulled from queue
_handlePacket :: emitting packetreceive
_handleAck :: packet type suback
ok 1 no error on subscribe
publish :: message `Hello World!` to topic `hello`
publish :: qos 0
MqttClient:publish: packet cmd: publish
_sendPacket :: (mqttjs_857bf1ef) ::  start
sendPacket :: packet: {
  cmd: 'publish',
  topic: 'hello',
  payload: 'Hello World!',
  qos: 0,
  retain: false,
  messageId: 0,
  dup: false
}
sendPacket :: emitting `packetsend`
sendPacket :: writing to stream
sendPacket :: writeToStream result true
sendPacket :: invoking cb
ok 2 no error on publish
_sendPacket :: (mqttjs_857bf1ef) ::  end
writable stream :: parsing buffer
work :: getting next packet in queue
work :: no packets in queue
work :: done flag is true
writable stream :: parsing buffer
work :: getting next packet in queue
work :: no packets in queue
work :: done flag is true
writable stream :: parsing buffer
work :: getting next packet in queue
work :: no packets in queue
work :: done flag is true
writable stream :: parsing buffer
work :: getting next packet in queue
work :: no packets in queue
work :: done flag is true
writable stream :: parsing buffer
parser :: on packet push to packets array.
work :: getting next packet in queue
work :: packet pulled from queue
_handlePacket :: emitting packetreceive
_handlePublish: packet {
  cmd: 'publish',
  retain: false,
  qos: 0,
  dup: false,
  length: 19,
  topic: 'hello',
  payload: {
    type: 'Buffer',
    data: [
      72,
      101,
      108,
      108,
      111,
      32,
      87,
      111,
      114,
      108,
      100,
      33,
      [length]: 12
    ]
  }
}
_handlePublish: qos 0
ok 3 should match topic
ok 4 should match payload
end :: (mqttjs_857bf1ef)
end :: cb? true
_clearReconnect : clearing reconnect timer
end :: (mqttjs_857bf1ef) :: immediately calling finish
end :: (mqttjs_857bf1ef) :: finish :: calling _cleanUp with force false
_cleanUp :: done callback provided for on stream close
_cleanUp :: forced? false
_cleanUp :: (mqttjs_857bf1ef) :: call _sendPacket with disconnect packet
_sendPacket :: (mqttjs_857bf1ef) ::  start
sendPacket :: packet: { cmd: 'disconnect' }
sendPacket :: emitting `packetsend`
sendPacket :: writing to stream
sendPacket :: writeToStream result true
sendPacket :: invoking cb
_cleanUp :: clearing pingTimer

Ok first problem seems that done is never called in _cleanUp because this check:

if (done && !this.connected) {

fails as this.connected = true. I dunno how that could be false as we don't set it to false anywhere in the code 🤷🏼‍♂️ Even if I bypass that by calling done when the disconnect packet is sent then the close event is never triggered as the stream close event is not triggered at all I admit I'm a bit lost now and dunno what to do

BertKleewein · 2023-07-13T16:48:55Z

@robertsLando - This looks like the same behavior I saw when patched mqtt-packet and readable-stream. Thanks for confirming that you see the same thing. It smells like it's maybe related to some of the client.end ugliness that I've been poking around. I'll take a look and see what I can find.

robertsLando · 2023-07-14T06:09:51Z

@BertKleewein I'm planning to do a PR that moves client pakcets handlers outside like aedes does: https://github.com/moscajs/aedes/tree/main/lib/handlers

Hope that will not be so hard BTW... My plans are to refactor client piece by piece without breaking too much stuff

robertsLando · 2023-07-14T07:15:42Z

Dunno if @mcollina could enlighten us on this, I'm not sure it's only a problem with MQTTjs, maybe it's something related to the readable-stream changed with stream or the setImmediate behaving different in browser env

…browserify

robertsLando · 2023-07-19T09:34:15Z

@getlarge ever used rollup ? :) I'm a bit lost here

getlarge · 2023-07-19T15:39:44Z

@getlarge ever used rollup ? :) I'm a bit lost here

Never did sorry! Still stuck ?
Also out of curiosity, is there a special reason to use rollup ?

robertsLando · 2023-07-20T12:53:22Z

@getlarge it seems to have better support for modern browsers

…browserify

…into rollup-browserify

robertsLando · 2023-08-02T08:22:15Z

Supersided by #1658

chore: replace browserify with rollup

1878f1b

robertsLando added 3 commits July 10, 2023 16:39

Merge branch 'main' of https://github.com/mqttjs/MQTT.js into rollup-…

c63e14f

…browserify

Merge branch 'main' into rollup-browserify

210783b

fix: improvements

7bc31ed

robertsLando mentioned this pull request Jul 12, 2023

Class this is set to super() instance rollup/rollup#5062

Closed

chore: readable-stream and mqtt-packet patches

e200ee2

robertsLando marked this pull request as ready for review July 13, 2023 08:10

Merge branch 'main' of https://github.com/mqttjs/MQTT.js into rollup-…

ad92a60

…browserify

chore: use newer node polifilly

e114439

robertsLando added 5 commits August 1, 2023 13:47

Merge branch 'main' of https://github.com/mqttjs/MQTT.js into rollup-…

7b1e057

…browserify

fix: json fixes

c22a902

fix: rollup entrypoint

6391d92

chore: bump deps

7182c6b

Merge branch 'main' into rollup-browserify

6a07c6e

robertsLando mentioned this pull request Aug 1, 2023

MQTT v5 Feedbacks #1639

Closed

fix: add esm support to airtap

008de94

Merge branch 'rollup-browserify' of https://github.com/mqttjs/MQTT.js …

ed09bf9

…into rollup-browserify

robertsLando closed this Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: replace `browserify` with `rollup` #1632

chore: replace `browserify` with `rollup` #1632

robertsLando commented Jul 7, 2023

codecov bot commented Jul 7, 2023 •

edited

Loading

robertsLando commented Jul 12, 2023 •

edited

Loading

robertsLando commented Jul 12, 2023 •

edited

Loading

vishnureddy17 commented Jul 12, 2023

BertKleewein commented Jul 12, 2023

robertsLando commented Jul 13, 2023 •

edited

Loading

robertsLando commented Jul 13, 2023 •

edited

Loading

robertsLando commented Jul 13, 2023 •

edited

Loading

BertKleewein commented Jul 13, 2023

robertsLando commented Jul 14, 2023

robertsLando commented Jul 14, 2023

robertsLando commented Jul 19, 2023

getlarge commented Jul 19, 2023 •

edited

Loading

robertsLando commented Jul 20, 2023

robertsLando commented Aug 2, 2023

chore: replace browserify with rollup #1632

chore: replace browserify with rollup #1632

Conversation

robertsLando commented Jul 7, 2023

codecov bot commented Jul 7, 2023 • edited Loading

Codecov Report

robertsLando commented Jul 12, 2023 • edited Loading

robertsLando commented Jul 12, 2023 • edited Loading

vishnureddy17 commented Jul 12, 2023

BertKleewein commented Jul 12, 2023

robertsLando commented Jul 13, 2023 • edited Loading

robertsLando commented Jul 13, 2023 • edited Loading

robertsLando commented Jul 13, 2023 • edited Loading

BertKleewein commented Jul 13, 2023

robertsLando commented Jul 14, 2023

robertsLando commented Jul 14, 2023

robertsLando commented Jul 19, 2023

getlarge commented Jul 19, 2023 • edited Loading

robertsLando commented Jul 20, 2023

robertsLando commented Aug 2, 2023

chore: replace `browserify` with `rollup` #1632

chore: replace `browserify` with `rollup` #1632

codecov bot commented Jul 7, 2023 •

edited

Loading

robertsLando commented Jul 12, 2023 •

edited

Loading

robertsLando commented Jul 12, 2023 •

edited

Loading

robertsLando commented Jul 13, 2023 •

edited

Loading

robertsLando commented Jul 13, 2023 •

edited

Loading

robertsLando commented Jul 13, 2023 •

edited

Loading

getlarge commented Jul 19, 2023 •

edited

Loading