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

WIP: Remove EventEmitter from roslib.js bundle #344

Closed
wants to merge 1 commit into from
Closed

WIP: Remove EventEmitter from roslib.js bundle #344

wants to merge 1 commit into from

Conversation

Rayman
Copy link
Contributor

@Rayman Rayman commented Sep 23, 2019

I'm not sure why, but EventEmitter is included within roslib.js. It
should be a dependency according to all the readmes. This change excludes it from the bundle.

name roslib.js roslib.min.js
old 131K 53K
new 104K 42K

Bundle size analysis here:
https://roslibjs-bundle-size-example.netlify.com

@Rayman Rayman mentioned this pull request Sep 24, 2019
Closed
@mvollrath
Copy link
Contributor

The output roslib.js works, but this breaks npm run publish tests for me.

$ nodejs --version
v8.10.0
$ npm --version
6.5.0-next.0
$ rm -rf node_modules/
$ npm install
...
$ npm run publish

Running "karma:build" (karma) task
11 02 2020 15:49:48.504:INFO [karma-server]: Karma v3.1.4 server started at http://0.0.0.0:9876/
11 02 2020 15:49:48.506:INFO [launcher]: Launching browsers Firefox with concurrency unlimited
11 02 2020 15:49:48.509:INFO [launcher]: Starting browser Firefox
11 02 2020 15:49:50.184:INFO [Firefox 72.0.0 (Ubuntu 0.0.0)]: Connected on socket iJ8qYUNqg_-R4eRuAAAA with id 85713544
Firefox 72.0.0 (Ubuntu 0.0.0) ERROR
  {
    "message": "Error: Cannot find module 'eventemitter2'\nat /home/matt/src/roslibjs/build/roslib.js:1:159\n\no@/home/matt/src/roslibjs/build/roslib.js:1:159\no/<@/home/matt/src/roslibjs/build/roslib.js:1:316\n[14]<@/home/matt/src/roslibjs/build/roslib.js:1967:28\no@/home/matt/src/roslibjs/build/roslib.js:1:265\no/<@/home/matt/src/roslibjs/build/roslib.js:1:316\n[13]<@/home/matt/src/roslibjs/build/roslib.js:1276:22\no@/home/matt/src/roslibjs/build/roslib.js:1:265\no/<@/home/matt/src/roslibjs/build/roslib.js:1:316\n[19]<@/home/matt/src/roslibjs/build/roslib.js:2459:10\no@/home/matt/src/roslibjs/build/roslib.js:1:265\no/<@/home/matt/src/roslibjs/build/roslib.js:1:316\n[4]<@/home/matt/src/roslibjs/build/roslib.js:601:16\no@/home/matt/src/roslibjs/build/roslib.js:1:265\no/<@/home/matt/src/roslibjs/build/roslib.js:1:316\n[5]</<@/home/matt/src/roslibjs/build/roslib.js:615:17\n[5]<@/home/matt/src/roslibjs/build/roslib.js:616:4\no@/home/matt/src/roslibjs/build/roslib.js:1:265\nr@/home/matt/src/roslibjs/build/roslib.js:1:432\n@/home/matt/src/roslibjs/build/roslib.js:1:460\n",
    "str": "Error: Cannot find module 'eventemitter2'\nat /home/matt/src/roslibjs/build/roslib.js:1:159\n\no@/home/matt/src/roslibjs/build/roslib.js:1:159\no/<@/home/matt/src/roslibjs/build/roslib.js:1:316\n[14]<@/home/matt/src/roslibjs/build/roslib.js:1967:28\no@/home/matt/src/roslibjs/build/roslib.js:1:265\no/<@/home/matt/src/roslibjs/build/roslib.js:1:316\n[13]<@/home/matt/src/roslibjs/build/roslib.js:1276:22\no@/home/matt/src/roslibjs/build/roslib.js:1:265\no/<@/home/matt/src/roslibjs/build/roslib.js:1:316\n[19]<@/home/matt/src/roslibjs/build/roslib.js:2459:10\no@/home/matt/src/roslibjs/build/roslib.js:1:265\no/<@/home/matt/src/roslibjs/build/roslib.js:1:316\n[4]<@/home/matt/src/roslibjs/build/roslib.js:601:16\no@/home/matt/src/roslibjs/build/roslib.js:1:265\no/<@/home/matt/src/roslibjs/build/roslib.js:1:316\n[5]</<@/home/matt/src/roslibjs/build/roslib.js:615:17\n[5]<@/home/matt/src/roslibjs/build/roslib.js:616:4\no@/home/matt/src/roslibjs/build/roslib.js:1:265\nr@/home/matt/src/roslibjs/build/roslib.js:1:432\n@/home/matt/src/roslibjs/build/roslib.js:1:460\n"
  }

@Rayman Rayman changed the title Remove EventEmitter from roslib.js bundle WIP: Remove EventEmitter from roslib.js bundle Feb 12, 2020
@Rayman
Copy link
Contributor Author

Rayman commented Feb 12, 2020

You are right, I broke the tests. I've now fixed them.

I've to check webpack next to make sure I'm not breaking anybody's workflow.

@mvollrath
Copy link
Contributor

It works now, but I'm not sure this should be done.

Removing EventEmitter from the bundle makes ROSLIB smaller, but you still have to obtain EventEmitter, so the overall application size is probably about the same and it's a separate HTTP request to a separate host rather than multiplexed. In my quick testing, this PR + going to a CDN for EventEmitter doubles the load time of the ROSLIB stack compared to using bundled EventEmitter on develop.

A better change might be to stop instructing people to load EventEmitter from CDN (or anywhere), because it's no longer required and is just slowing things down. Bundling the dependency is more convenient and faster for most users, I think.

@Rayman
Copy link
Contributor Author

Rayman commented Feb 27, 2020

So the problem I have is that I'm also using EventEmitter in my own code and so the library will have to be loaded twice. This is wasted data.

With this PR I'm only having to download one copy. This is how it used to work before 5d13337.

@MatthijsBurgh MatthijsBurgh force-pushed the fix/browserify-build branch 4 times, most recently from f5209d3 to f12a1db Compare April 10, 2021 08:25
@MatthijsBurgh MatthijsBurgh force-pushed the fix/browserify-build branch 2 times, most recently from cd7040e to c16db14 Compare November 1, 2022 13:09
@EzraBrooks
Copy link
Contributor

Converting this PR to a draft, as the title indicates it's WIP.

@EzraBrooks EzraBrooks marked this pull request as draft October 26, 2023 18:15
@Rayman @MatthijsBurgh
Remove EventEmitter from roslib.js bundle
1b948c8 
I'm not sure why, but EventEmitter is included within roslib.js. It
should be a dependency according to all the readmes. This change excludes it from the bundle.

| name | roslib.js | roslib.min.js |
|------|-----------|---------------|
| old  | 132K      | 53K           |
| new  | 107K      | 42K           |
@MatthijsBurgh
Copy link
Contributor

@EzraBrooks I see you marked this as a draft. But what do you think of this PR? I think this is a relevant PR to consider, when we want to improve library quality.

I do know @Rayman, so I know he is open to discuss/explain when needed.

@EzraBrooks
Copy link
Contributor

I definitely agree with this from a philosophical perspective, but won't it break the CDN-served scripts? Might require a major version bump.

@Rayman
Copy link
Contributor Author

Rayman commented Nov 17, 2023

@MatthijsBurgh in this PR #615 you removed loading eventemitter from the CDN. This means the bundled eventemitter is now used. So this PR will break the examples.

Originally this package was designed that you first load eventemitter2, then roslib in a script tag. This makes both files easily cachable and small.

@MatthijsBurgh MatthijsBurgh added the v2 Issues and PRs, which will be fixed in v2, as it is a breaking change label Nov 28, 2023
@MatthijsBurgh
Copy link
Contributor

Lets include this in v2.

@MatthijsBurgh
Copy link
Contributor

This will indeed require adding back loading the eventemitter CDN in the examples

@MatthijsBurgh MatthijsBurgh mentioned this pull request Dec 19, 2023
Closed
@EzraBrooks EzraBrooks mentioned this pull request Feb 20, 2024
Merged
@EzraBrooks
Copy link
Contributor

Finally going to get this change in via #688 🙂

@EzraBrooks EzraBrooks closed this Feb 20, 2024
@EzraBrooks EzraBrooks added this to the 2.0.0 milestone Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
v2 Issues and PRs, which will be fixed in v2, as it is a breaking change
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Rayman @mvollrath @EzraBrooks @MatthijsBurgh