Yomguithereal / mnemonist

Curated collection of data structures for the JavaScript/TypeScript language.
https://yomguithereal.github.io/mnemonist
MIT License
2.24k stars 92 forks source link

fix: add missing constructor to TS type definitions #213

Closed jerome-benoit closed 6 months ago

jerome-benoit commented 6 months ago

TS with Node16 or NodeNext module target expect explicit constructor to to allow new Foo() without syntax error.

Yomguithereal commented 6 months ago

@jerome-benoit this looks sensible and I will probably merge and release soon, but do you have some documentation or reference regarding this issue so I can document myself on the problem here? Is this a recent change wrt TS?

jerome-benoit commented 6 months ago

I've not been gone through the TS release notes, but I noticed the error while switching a repo https://github.com/SAP/e-mobility-charging-stations-simulator to ESM ("type": "module" in package.json and "module": "NodeNext" in tsconfig.json)

I think it's more an ESM related requirements than a TS behavioral change.

=> import Queue from 'mnemonist/queue.js' makes tsc and eslint fail with various errors: missing constructor and typing incorrect on methods
=> import { Queue } from 'mnemonist' makes tsc fail with one error: missing constructor

And the resulting build with esbuild in both case fail to load the module if mnemonist is in external.

Yomguithereal commented 6 months ago

@jerome-benoit merged and released as part of v0.39.7.

Since I have you here and since you seem to be well versed in ESM stuff, do you know if the "schism" is over and if there is a completely sure and not bonkers way to support modern ESM-first packaging all while ensuring perfect backwards compatiblity with UMD/require and older node versions etc. ?

jerome-benoit commented 6 months ago

package.json permits to define the default exports for librairies willing to support CommonJS, ESM and TypeScript:

 "exports": {
    ".": {
      "types": "./lib/index.d.ts",
      "require": "./lib/index.cjs",
      "import": "./lib/index.mjs"
    }
  },

For individual module exports, import a CommonJS module into ESM module works. The other way around is not intended to work per design: ESM is targeted to replace CommonJS.

Most libraries just use an ESM wrapper for the CommonJS default exports like export * from './index.(c)js'.

TS seems to be more strict regarding file extensions: .cjs does not seem to be searched by default. I've broken CommonJS support lately in a library I maintain: https://github.com/poolifier/poolifier/issues/1821

jerome-benoit commented 6 months ago

That PR is not enough to make mnemonist fully support ESM with TS. I will make another PR shortly to make mnemonist fully ESM compliant via a default wrapper for exports. I do not think making individual modules ESM compliant via wrappers worth the effort, the default exports file is enough.