antlr4
antlr4 copied to clipboard
fix(javascript): fix types not being recognized for NodeNext module resolution
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
.
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.
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:
After:
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.
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.
@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:
Maybe you want to try that out? I'm open to accept a PR to fix the last remaining issue.
@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.
@parrt let's see if we can get that one in
@partt blessed
Looks like there are conflicts.
@parrt I see conflicts if you rebase, not if you merge ?