feat: add ESM support

[unional]

I kept the existing code unchanged and only add the new index.mjs file copied from index.js.

Adjusted var => const and let,
and update package.json to with the exports field and added test:esm script.

Updated ci.yml to run npm run test:esm.

Fixes #251 #248

[lpinca]

Can't we use a wrapper as documented here https://nodejs.org/api/packages.html#approach-1-use-an-es-module-wrapper instead of duplicating everything?

[unional]

AFAIK, that doesn't work in practice.
For example, if you try to run test using jest, you will end up with error "exports is not defined" because the CJS code using module.exports.

I can give it a try to see how it works out.

[unional]

Good news. I have tried it and it works.

The reason it works is the index.js did the check of if ('undefined' === typeof module) so the code is not hit in ESM pipeline.

Also updated the index.mjs and index.d.ts to export EventEmitter as named export, to match the usage of import { EventEmitter } from 'eventemitter3'

[lpinca]

Another issue I see is that this works in Node.js but not in the browser. We only have the UMD for the browser.

[unional]

Another issue I see is that this works in Node.js but not in the browser. We only have the UMD for the browser.

I think UMD is ok, people who use UMD are not using ESM. So the code can remain as is.

[lpinca]

I think UMD is ok, people who use UMD are not using ESM. So the code can remain as is.

Yes I was referring to the fact that is not possible to use this as ESM in the browser. For example using a CDN like https://www.jsdelivr.com/. I wonder if it makes sense to create both an ESM and UMD module renaming the umd folder to dist and using rollup instead of browserify.

[unional]

Yes I was referring to the fact that is not possible to use this as ESM in the browser.

I see, so you want to have two bundles, one for umd and one for esm.

create both an ESM and UMD module renaming the umd folder to dist

That will be a braking change for require('.../umd/eventemitter3.js') unless we hack it in the exports map. 😛

[lpinca]

I see, so you want to have two bundles, one for umd and one for esm.

Ideally yes, that is what I would expect from "ESM support".

[lpinca]

That will be a braking change for require('.../umd/eventemitter3.js') unless we hack it in the exports map.

There are already breaking changes.

[unional]

Ideally, yes, that is what I would expect from "ESM support".

Ok. let me see if I can cook up one.

[unional]

There are already breaking changes.

If that's the case, do you still want to support the use case of require('../dist/eventemitter3.js')?

I don't see why people would do deep linking in NodeJS env.

[unional]

One more question. Do you want to also replace uglifyjs? And do you want to keep the minified code in ES5 or we can use terser instead?

UPDATE: I have updated to use terser. Please let me know if you have any concerns

[unional]

There is some problem with the adjustment of:

import EventEmitter from './index.js'
// to
import * as EventEmitter from './index.js`

in the index.mjs.

First one works, second doesn't. But rollup complain about the first one. Need to fix that

UPDATE: done

[lpinca]

Thanks. I will review properly in the next few days.

[unional]

All review changes updated and squashed for easier reading.

[lpinca]

Thank you.