FredKSchott / rollup-plugin-polyfill-node

A modern Node.js polyfill for your Rollup bundle.
Other
177 stars 57 forks source link

Bad event polyfills #22

Closed adgang closed 2 years ago

adgang commented 3 years ago

Event polyfills look like this:

var events = /*#__PURE__*/Object.freeze({
  __proto__: null,
  'default': EventEmitter,
  EventEmitter: EventEmitter
});

I believe this should not be the behaviour.

Please see this discussion for details: https://github.com/snowpackjs/snowpack/discussions/1480

quolpr commented 3 years ago

So, I investigated that polyfill is incorrect. Here how node makes export - https://github.com/nodejs/node/blob/v16.2.0/lib/events.js#L77

And I am not sure how to fix it. Cause as I understand, polyfill uses ESM, but other packages use CJS style(through require). And I am not sure how to make that ESM export(that is used in this lib, https://github.com/snowpackjs/rollup-plugin-polyfill-node/blob/main/polyfills/events.js#L14 ) work like the CJS export(so there are no default when you make require, just direct object).

I mean that when you make require('event') - the EventEmitter should be returned, instead of

{
  'default': EventEmitter,
  EventEmitter: EventEmitter
}
quolpr commented 3 years ago

@FredKSchott could you check my thoughts? If I am right, this is a pretty serious issue. And it seems to be connected with https://github.com/snowpackjs/rollup-plugin-polyfill-node/issues/23 .

simonhaenisch commented 2 years ago

Just dropping this here because I also had issues with the events polyfill... only in a native environment though, still not sure what the cause was. The error I got was

TypeError: Object is not a constructor (evaluating 'new EventEmitter()')

I ended up switching to https://github.com/browserify/events which works as expected (used by Webpack as well), and setting preferBuiltins: false for the node-resolve plugin.

npm install events
export default {
  // ...
  plugins: [
    commonjs(),
    nodeResolve({ preferBuiltins: false }),
    // you can probably still use `polyfillNode()` after this for other polyfills,
    // because `events` will have been resolved from the installed package already
    // (in our case we only needed the events polyfill)
  ]
}
FredKSchott commented 2 years ago

Fixed by https://github.com/FredKSchott/rollup-plugin-polyfill-node/pull/42