Skip to content

Externalize EventEmitter3 dependency#688

Merged
EzraBrooks merged 9 commits intodevelopfrom
externalize-eventemitter3-dependency
Mar 20, 2024
Merged

Externalize EventEmitter3 dependency#688
EzraBrooks merged 9 commits intodevelopfrom
externalize-eventemitter3-dependency

Conversation

@EzraBrooks
Copy link
Copy Markdown
Contributor

@EzraBrooks EzraBrooks commented Feb 20, 2024 •
edited
Loading

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 EzraBrooks mentioned this pull request Feb 20, 2024
Closed
@EzraBrooks EzraBrooks added the v2 Issues and PRs, which will be fixed in v2, as it is a breaking change label Feb 20, 2024
@EzraBrooks EzraBrooks force-pushed the externalize-eventemitter3-dependency branch from 0a20c86 to 74bfef6 Compare February 20, 2024 19:03
@EzraBrooks EzraBrooks force-pushed the externalize-eventemitter3-dependency branch from 74bfef6 to 5884325 Compare February 20, 2024 19:43
@EzraBrooks
Copy link
Copy Markdown
Contributor Author

EzraBrooks commented Feb 20, 2024

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
Remove all remaining Grunt tasks in favor of Vite configs
69ee2bb 
- Remove browserify/uglify in favor of Vite in library mode.
- Replace grunt-jsdoc with an npm script
- Replace grunt-eslint with vite-plugin-checker
- Replace invocations of `tsc` with vite-plugin-checker
- Use dynamic imports for browser/node.js environment differences
- Remove various Grunt plugins
@EzraBrooks EzraBrooks force-pushed the externalize-eventemitter3-dependency branch from 5884325 to 05dea17 Compare February 20, 2024 23:56
Base automatically changed from switch-to-vite to develop February 21, 2024 08:09
@EzraBrooks
Copy link
Copy Markdown
Contributor Author

EzraBrooks commented Feb 21, 2024

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 EzraBrooks mentioned this pull request Mar 18, 2024
Merged
@EzraBrooks
Copy link
Copy Markdown
Contributor Author

EzraBrooks commented Mar 18, 2024

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
Copy link
Copy Markdown
Contributor

MatthijsBurgh commented Mar 19, 2024

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

@EzraBrooks
Copy link
Copy Markdown
Contributor Author

EzraBrooks commented Mar 19, 2024

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
Copy link
Copy Markdown
Contributor

MatthijsBurgh commented Mar 19, 2024

Sounds good to me

@EzraBrooks EzraBrooks marked this pull request as ready for review March 20, 2024 17:20
@EzraBrooks EzraBrooks force-pushed the externalize-eventemitter3-dependency branch from 83d052d to 6f52736 Compare March 20, 2024 17:36
@EzraBrooks
Copy link
Copy Markdown
Contributor Author

EzraBrooks commented Mar 20, 2024

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

@EzraBrooks EzraBrooks merged commit bbe89a6 into develop Mar 20, 2024
@EzraBrooks EzraBrooks deleted the externalize-eventemitter3-dependency branch March 20, 2024 20:49
@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

@MatthijsBurgh MatthijsBurgh MatthijsBurgh approved these changes

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.

2 participants

@EzraBrooks @MatthijsBurgh