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.

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

Update version of events bundled #1907

Open
patrickroberts opened this issue Apr 26, 2019 · 12 comments
Open

Update version of events bundled #1907

patrickroberts opened this issue Apr 26, 2019 · 12 comments

Comments

@patrickroberts
Copy link

Browserify currently uses events@^2.0.0, but events has an updated version 3.0.0 to support the introduction of the off() method introduced in Node v10. Is there a way to override the version of events that browserify uses for bundling? I also think browserify should bump the version used on package.json, but note that events@3.0.0 no longer supports ES5 shims out of the box.

@ljharb
Copy link
Member

ljharb commented Apr 26, 2019

I’m confused why adding off was semver major.

@patrickroberts
Copy link
Author

@ljharb because that was not the only change. As I pointed out, they removed support for ES5 shims, which is a breaking change.

@ljharb
Copy link
Member

ljharb commented Apr 26, 2019

Then i think off should be backported to events v2, so that browserify doesn’t need to update to v3 and drop support.

@patrickroberts
Copy link
Author

Meanwhile, my secondary question is how can I run browserify and override the version of events used for bundling?

@ljharb
Copy link
Member

ljharb commented Apr 26, 2019

If it’s a dep of browserify, then npm doesn’t provide that feature, so I’m not sure how :-/

@patrickroberts
Copy link
Author

I'll open an issue on events then. Thanks for the quick responses

@goto-bus-stop
Copy link
Member

goto-bus-stop commented Apr 26, 2019

If you install events@3 with npm you can do

browserify -r events/:events

The events/ bit forces browserify to use your locally installed version, and :events aliases it to "events", replacing it throughout the bundle.

Node.js treated .off as a major version bump, so I did the same for events.

There is a PR to update our dependency at #1839 that i've been meaning to batch with different changes, so we don't churn through browserify major versions too much. I haven't gotten around to those for a long time though so we should just go ahead with v17 soon with the PRs that are already there

@patrickroberts
Copy link
Author

@goto-bus-stop sorry, I just saw this comment after posting the issue on browserify. Thank you! I'm just trying to fix typed-scheduler because when I published, I didn't realize the dst/ was broken. I plan on adding unit testing soon but I haven't gotten around to that just yet.

@ljharb
Copy link
Member

ljharb commented Apr 26, 2019

node was being overly conservative because of the likelihood of people polyfilling and inheriting from it; it needn’t be major imo.

@patrickroberts
Copy link
Author

@goto-bus-stop sorry to bug you again but I couldn't get that command line flag working with the other options I had. I currently have this:

rm -f dst/*
browserify --debug \
  --entry src/scheduler.ts \
  --plugin [tsify --project .] \
  --standalone Scheduler | exorcist \
  dst/scheduler.js.map > dst/scheduler.js
uglifyjs dst/scheduler.js \
  --output dst/scheduler.min.js --compress --mangle --source-map \
  "base='dst',content='dst/scheduler.js.map',url='scheduler.min.js.map'"

which produces what I want, but with the wrong events module. I have events@3.0.0 installed in my project's devDependencies (I publish the dst/ folder on npm), and when I add the --require events/:events \, the UMD module sets events as the entry point instead of scheduler for both scheduler.js and scheduler.min.js:

rm -f dst/*
browserify --debug \
  --entry src/scheduler.ts \
  --plugin [tsify --project .] \
  --require events/:events \
  --standalone Scheduler | exorcist \
  dst/scheduler.js.map > dst/scheduler.js
uglifyjs dst/scheduler.js \
  --output dst/scheduler.min.js --compress --mangle --source-map \
  "base='dst',content='dst/scheduler.js.map',url='scheduler.min.js.map'"

Am I using incompatible options or something?

@goto-bus-stop
Copy link
Member

Ahh I see, I think there is an old bug with combined use of -r and -s. I thiiink this PR is related browserify/browser-pack#51

I see you've switched the repo to use Rollup now, that is a better fit anyway—the main field in package.json should refer to a CommonJS module, and while browserify --standalone technically works as a CJS module, it's not intended to be used as part of an end user bundle. Rollup can output proper CJS modules that can be easily consumed by bundlers.

@patrickroberts
Copy link
Author

I did switch to rollup, but I actually encountered the same issue there with the version of events that rollup-plugin-node-builtins uses, so I ended up reverting off() to removeListener() anyway. I'm glad to see at least this issue was not simply some stupidity on my part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants