Rework TypeScript .d.ts handling #978
Conversation
This reworks .d.ts handling to address the issues in chartjs#977 and reported by [Are the Types Wrong](https://arethetypeswrong.github.io/?p=chartjs-plugin-annotation%403.1.0). Background information: * According to the [TypeScript Handbook](https://www.typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html#im-writing-a-library): > If you’re using a bundler to emit your library... You must ensure that your declaration files get bundled as well. Recall the [first rule of declaration files](https://www.typescriptlang.org/docs/handbook/modules/theory.html#the-role-of-declaration-files): every declaration file represents exactly one JavaScript file. * According to [Are the Types Wrong](https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseESM.md), > A golden rule of declaration files is that if they represent a module—that is, if they use import or export at the top level—they must represent exactly one JavaScript file. They especially cannot represent JavaScript files of two different module formats. * chartjs-plugin-annotation's CJS build uses a CJS default export (see [Rollup's docs](https://rollupjs.org/configuration-options/#output-exports)). Properly representing that in a `.d.ts` file requires using `export = Annotation`, but the `export =` syntax both raises challenges for exporting types alongside the default export and is incompatible with ESM. (The [TypeScript Handbook](https://www.typescriptlang.org/docs/handbook/modules/appendices/esm-cjs-interop.html) has even more background information here. See also [tshy's documentation](https://github.com/isaacs/tshy?tab=readme-ov-file#handling-default-exports), "`export default`` is the bane of hybrid TypeScript modules" - although I believe that the situation is [not quite as bad as it says](isaacs/tshy#105).) Pulling this together for chartjs-plugin-annotation: * Use [rollup-plugin-dts](https://www.npmjs.com/package/rollup-plugin-dts) to bundle declaration files alongside the bundled library files (as recommended by the TypeScript Handbook). * Invoke rollup-plugin-dts twice to create separate `.d.ts` files for the ESM and CJS builds (and update package.json to reference those). * Although a CJS file with a default export can't export additional values, it is apparently possible to export types alongside the default value, if the types are placed in a namespace with the same name as the default value. * I had trouble finding a solution for the `export =` default issue. (I couldn't get rollup-plugin-dts to create an `export =` style definition. Changing chartjs-plugin-annotation to not use a CJS default export may be the simplest long-term solution, but it would break backward compatibility.) To solve this, I added @rollup/plugin-replace to apply the necessary namespace and `export =` statement via string replacement. Fixes chartjs#977
|
I did some further investigation into the
For the purposes of this PR, I went with approach 2, but please let me know how you'd like to proceed. If chartjs-plugin-annotation ends up making a breaking change, I wonder if it would be best to separate the CJS and unminified UMD builds, as Chart.js itself does, to make this a little more explicit. |
|
A fourth approach is to create a separate .d.ts file for the CJS version that re-exports everything. See chartjs/chartjs-plugin-datalabels#436. chartjs-plugin-annotation defines enough data types that this is a decent amount of duplication. |
|
To me, aligning with chart.js style sounds like a good option in the long run. Would not be a huge breaking change, but a breaking change anyway. |
|
@kurkle , do you want me to pursue that as part of this PR? |
|
@joshkel not quite sure, I'll need to catch up first. @stockiNail, @etimberg thoughts? |
|
Catching up as well. My gut says that we should align with the Chart.js types even if it's breaking now |
|
Apologize for my late reply. Well, I have to admit that during last 2 years I have never touched any javascript code in deep therefore I'm a bit out of the problem. Anyway
and
I fully agree. Working in the past on chart.js, this plugin and others, I often see this differences, on packaging as well. In my opinion could make sense to align all in chartjs org even if it could be a breaking change. |
This reworks .d.ts handling to address the issues in #977 and reported by Are the Types Wrong.
Background information:
According to the TypeScript Handbook:
According to Are the Types Wrong,
chartjs-plugin-annotation's CJS build uses a CJS default export (see Rollup's docs). Properly representing that in a
.d.tsfile requires usingexport = Annotation, but theexport =syntax both raises challenges for exporting types alongside the default export and is incompatible with ESM. (The TypeScript Handbook has even more background information here. See also tshy's documentation, "export defaultis the bane of hybrid TypeScript modules" - although I believe that the situation is not quite as bad as it says.)Pulling this together for chartjs-plugin-annotation:
.d.tsfiles for the ESM and CJS builds (and update package.json to reference those).export =default issue. (I couldn't get rollup-plugin-dts to create anexport =style definition. Changing chartjs-plugin-annotation to not use a CJS default export may be the simplest long-term solution, but it would break backward compatibility.)To solve this, I added @rollup/plugin-replace to apply the necessary namespace and(changed - see below)export =statement via string replacement.Fixes #977