search

RobotWebTools / roslibjs

The Standard ROS JavaScript Library
https://robotwebtools.github.io/roslibjs
Other
659 stars 372 forks source link

Externalize EventEmitter3 dependency #688

Closed EzraBrooks closed 3 months ago

EzraBrooks commented 4 months ago

Public API Changes

Removes EventEmitter3 from the browser bundle. Browser users not using a bundler will need to globally include their own copy of EventEmitter3.

Description

Supersedes #344.

UMD bundle size before removal: 77.51kB. UMD bundle size after removal: 74.88kB.

EzraBrooks commented 4 months ago

FYI I'm headed out on vacation for a week but I'm cool with this and the other PR getting merged while I'm gone if they're good to go.

EzraBrooks commented 4 months ago

Maybe before we merge this we should add an integration test for a typical pure-frontend workflow that includes the library via a script tag.

EzraBrooks commented 3 months ago

I think this PR is working (if I manually look at the bundled output) but I'm having trouble making a unit test verifying that EventEmitter isn't bundled, because the UMD bundle is automagically pulling in eventemitter3 via Node require() in the testing environment.

MatthijsBurgh commented 3 months ago

@EzraBrooks Is there maybe a way to check the contents of the bundle as unit test?

EzraBrooks commented 3 months ago

I could maybe write a test that opens the JS file as a text file and checks for some text we would know to exist if EventEmitter is present, like .on=function, for example.

MatthijsBurgh commented 3 months ago

Sounds good to me

EzraBrooks commented 3 months ago

looks like this is finally good to go: I added two tests, one to prove that the ROSLIB global exists, and one to verify EventEmitter is not in the bundle, and I'm now able to run the examples locally after adding the EventEmitter3 CDN reference to them