antlr4 icon indicating copy to clipboard operation
antlr4 copied to clipboard

fix(javascript): fix types not being recognized for NodeNext module resolution

Open KurtGokhan opened this issue 1 year ago • 5 comments

Fixes #4218

The problem was, since the package.json has type: module, Typescript is treating regular d.ts files as module. But the d.ts files are generated manually and they don't comply with how a module file should look like (e.g. missing file extensions on imports). So they are essentially more like CommonJS files.

Luckily, there is a way to override the module detection. If the file has cts or cjs extension, Typescript will treat that file as CommonJS. In this case, adding cts extension only to the index file was enough.

Ideally, the TS files should be built with tsc in the future to ensure the files comply with the corresponding module format.

Also added a files field to the package.json so that only relevant files are included in the NPM bundle. Previously it also included files like spec, src, and dev config files, which made it confusing to debug.

Tested my changes locally with various moduleResolution options like Node, Node10, Node16, NodeNext, Bundler and they work flawlessly. Only the Classic option doesn't work but it also doesn't work on upstream and it doesn't work for most other packages as well.

Alternative solutions

All of the other solutions require changes in more files but I will leave them here as an option.

The first obvious solution is to add extensions to all the TS files.

I checked various other packages like lodash-es and checked why they don't have this problem even though they don't have extension in TS files. Turns out, they don't have type: module in their package.json, so files are treated as CommonJS by default, even though the packages are ES packages.

So one of the solutions is to remove type: module and convert all js files to mjs.

KurtGokhan avatar Feb 23 '24 15:02 KurtGokhan

Interesting, I'd never heard of cts files before, thanks for this. See my last comment in #4218, hopefully we can get close to a packaging implementation that we can trust.

ericvergnaud avatar Feb 24 '24 11:02 ericvergnaud

Hopefully we can get close to a packaging implementation that we can trust.

Is the test suite for that absolutely necessary? Imho, we find a working configuration, test it manually in different scenarios, iterate if necessary. This isn't something that will change much. Most npm projects work with this approach.

Also, these changes should not affect runtime at all. This is only a Typescript change, and if needed we can test it with a simple setup of running tsc against different config options. Webpack shouldn't be needed.

Btw, sharing the results from AreTheTypesWrong:

Before: image

After: image

Here are the problems:

  • Internal Resolution Error can cause issues if skipLibCheck is false. But this seems to also exist on current setup. But I will check if it can be remedied.
  • Masquerading as CJS isn't a big issue since this package doesn't have any default exports.

KurtGokhan avatar Feb 24 '24 13:02 KurtGokhan

The lack of automated testing is the reason we are where we are. Every now and then, someone comes with a solution to a problem specific to their usage, whilst in parallel the ecosystem keeps evolving. We implement that change and that creates a problem for another use case.

ericvergnaud avatar Feb 24 '24 21:02 ericvergnaud

@KurtGokhan interesting tool you used for this information here. I didn't know that before.

As a test I let it check the new ANTLR4 TypeScript target antlr4ng and it came out as:

Bildschirmfoto 2024-04-18 um 21 38 03

Maybe you want to try that out? I'm open to accept a PR to fix the last remaining issue.

mike-lischke avatar Apr 18 '24 19:04 mike-lischke

@mike-lischke "Masquerading as ESM" shouldn't cause any problems as long as you are not using default exports. It may not need a fix. I will try out your implementation too, thanks.

KurtGokhan avatar Apr 18 '24 21:04 KurtGokhan

@parrt let's see if we can get that one in

ericvergnaud avatar Jul 25 '24 19:07 ericvergnaud

@partt blessed

ericvergnaud avatar Jul 28 '24 11:07 ericvergnaud

Looks like there are conflicts.

parrt avatar Jul 28 '24 18:07 parrt

@parrt I see conflicts if you rebase, not if you merge ?

ericvergnaud avatar Jul 28 '24 19:07 ericvergnaud