Package cannot be imported for certain TypeScript configurations

[nwalters512]

Describe the bug

I apologize in advance for filing this; ESM/CJS/TypeScript compatibility is a horrific can of worms.

Importing this package in a TypeScript project that uses "module": "Node16" fails when the project itself is not configured to use ESM (that is, package.json does not contain "type": "module").

I've created a reproduction here: https://github.com/nwalters512/sql-formatter-typescript-repro. Instructions are in the readme.

Expected behavior

Given that sql-formatter provides both ESM and CJS implementations, I would expect to be able to import the CJS version in this situation.

Actual behavior

TypeScript produces the following error:

src/index.ts:1:24 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("sql-formatter")' call instead.
  To convert this file to an ECMAScript module, change its file extension to '.mts', or add the field `"type": "module"` to '/Users/nathan/git/sql-formatter-typescript-repro/package.json'.

1 import { format } from 'sql-formatter';
                         ~~~~~~~~~~~~~~~


Found 1 error in src/index.ts:1

Note that things still work correctly at runtime; the presence of the types field combined with "type": "module" in this package confused TypeScript enough that it doesn't realize that there is a completely valid CJS implementation. I considered filing an issue upstream on TypeScript itself, but this was already discussed on microsoft/TypeScript#54263, which was closed without any indication that TypeScript itself would fix this (even though I think they should).

I believe the way to fix this would be to drop the "types" field from "exports" and instead place *.d.ts/*.d.cts files adjacent to the files that are referenced by the "import" and "require" exports in package.json. This way, TypeScript will automatically discover the correct types based on the particular conditions that it's using to resolve the implementation files.

[mgcrea]

After having encountered this with one of my packages I can confirm that just removing the exports.types field resolves the TypeScript error (TS1479).

[nene]

Sorry for taking this long to respond to this bug report.

You're right in saying that this whole topic is a big can of warms and I haven't felt like sticking my toes into it again, as each time I do I feel like I get smacked in the face :)

I tried the approaches you suggested, but I'm not sure that they worked out. I released multiple beta-versions of SQL Formatter:

• 14.0.0-beta.1 simply drops the "types" field from "exports".
• 14.0.0-beta.2 moves the *.d.ts files to the same directory as the *.js files.
• 14.0.0-beta.3 re-introduces the "types" field, but now it points to ./lib/index.d.ts instead of ./lib/src/index.d.ts.

Trying all these versions against your test repository I wasn't really happy with none of them.

Removing the types field does result in the tsc build succeeding without errors, but when opening the index.ts file in the editor (in my case VSCode), it's unable to pick up type definitions when importing "sql-formatter" package. Even when all the .d.ts files are alongside their respective .js files (as in beta-2) I'm getting the following error:

Could not find a declaration file for module 'sql-formatter'. '/Users/nene/github/sql-formatter-typescript-repro/node_modules/sql-formatter/dist/sql-formatter.min.cjs' implicitly has an 'any' type.

When I also include the "types" field to exports (as in beta-3) I get a more descriptive error:

The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("sql-formatter")' call instead.
  To convert this file to an ECMAScript module, change its file extension to '.mts', or add the field `"type": "module"` to '/Users/nene/github/sql-formatter-typescript-repro/package.json'.

Adding the type: "module" to package.json resolves the type info import problem in all the betas and also fixes the build error when using e.g. sql-formatter 12.x.

So I'm not really sure about the best approach here. Not having the types field fixes the tsc build issue, but leaves you with a quite confusing error message in the editor. In all cases defining type: "module" seems to be the only way to get everything properly working.

[nene]

Interestingly though for my own purposes I'm importing sql-formatter in a VSCode plugin and in that repository, which doesn't use type: "module", the beta.1 variant works fine in the editor and also compiles fine, while beta.2 and beta.3 fail with TypeScript unable to locate type definitions.

[nwalters512]

Thanks for following up on this, and for taking the time to play around with solutions! To respond to them:

• Just removing the types field will definitely cause problems. While you point out that the build with tsc succeeds, this is only because I didn't enable strict mode. I've updated the repro repo with "strict": true and with 14.0.0-beta.1, yarn build now fails (as expected).
• 14.0.0-beta.2 won't work because you still have a types field in package.json that points to lib/src/index.d.ts, which doesn't actually exist in this version. However, even after removing that, it won't work because TypeScript is looking for node_modules/sql-formatter/dist/sql-formatter.min.d.cts (adjacent to node_modules/sql-formatter/dist/sql-formatter.min.cjs). If you configure your tooling to output a file to dist/sql-formatter.min.d.cts, things should just work. That said, it looks like you're using webpack to produce dist/sql-formatter.min.cjs; I don't know how to get TypeScript to output a bundled type definition file for that. You may consider looking at a tool like tsup (https://github.com/egoist/tsup), which I've used in the past to take care of a lot of tricky bundling and type generation issues like this.
• 14.0.0-beta.3 won't work either because this is effectively the same situation that v13 is already in: by pointing to a .d.ts file in a project with "type": "module", you're telling it that the file that will be loaded at runtime is ESM, which is not the case. I don't think there's any solution to this problem that involves changing the single "types" field in package.json.

If you're open to it, I'd be willing to open a PR attempting to replace webpack with tsup, which I think would solve this problem. See https://tsup.egoist.dev/#generate-declaration-file and https://tsup.egoist.dev/#bundle-formats for how this can work. The downside is that the current setup where lib/ contains separate non-bundled files is actually quite nice, and that's lost when bundling.

[nene]

Sure. Feel free to experiment and make a pull request.

I'm not too worried about giving up separate files as long as one can still only import what he wants and rely on tree-shaking to eliminate the unused code.

[nwalters512]

I gave tsup a try today. While it does resolve the types issue, the usage of export * in src/index.ts results in ESBuild generating code that cannot be tree-shaken.

How attached are you to the CJS code (currently dist/sql-formatter.min.cjs) being bundled and minified? If we can avoid bundling entirely, I think we could have tsc itself generate CJS and ESM files directly. We can still generate dist/sql-formatter.min.cjs with webpack for unpkg.

[nene]

Thanks for taking a look.

the usage of export * in src/index.ts results in ESBuild generating code that cannot be tree-shaken.

We can throw the export * away if it simplifies things. It has been convenient to have it, but we can easily live without it.

How attached are you to the CJS code (currently dist/sql-formatter.min.cjs) being bundled and minified?

Not particularly attached. It has again been convenient to use it through unpkg in the demo page, but it's definitely not a must-have.

[nene]

@nwalters512 Thanks for your efforts.

I merged in your PR and released as 14.1.0-beta.1. My first tests with importing it inside the vscode extension look promising. Planning to do a bit more testing on it.

One thing that caught my eye is that we're outputting .d.ts files for the whole repo into the lib/ dir, but it seems like these files there are not in fact used at all. So I excluded that dir from npm package and did another release: 14.1.0-beta.2. This too seems to work.

I will be going away from computer for a week or so. So I probably won't place it into an official release just yet. But do play around with it and see if there's anything still missing.

[nene]

Looks like this issue has been left sitting here for a long time, while in fact the changes in it were included to final 15.0.0 release.

However now it seems that this whole change has caused the tree-shaking to be broken. See #836 :(