antlr4 icon indicating copy to clipboard operation
antlr4 copied to clipboard

Can't import from TypeScript runtime with `"moduleResolution": "nodenext"`

Open jedwards1211 opened this issue 1 year ago • 44 comments

I'm honestly not sure what needs to change in the package.json because I thought "type": "module", "main" and "types" would be enough, but apparently not. The ESM situation is a mess. When i turn on "moduleResolution": "nodenext", I get errors like

test.ts:1:22 - error TS2305: Module '"antlr4"' has no exported member 'CommonTokenStream'.

1 import { CharStream, CommonTokenStream }  from 'antlr4';

It works with "moduleResolution": "node", but according to TS docs that's really designed for CJS and doesn't support packages with "export" maps, so I wouldn't be able to use antlr4 and packages with export maps in the same project.

jedwards1211 avatar Apr 03 '23 02:04 jedwards1211

Probably related https://github.com/antlr/grammars-v4/issues/3314

iSuslov avatar Apr 03 '23 05:04 iSuslov

Hi, agreed that js/ts packaging world is a mess.

tsconfig.json: module = esnext moduleResolution = node

package.json type = module

this builds an esm library that is then successfully used by another esm ts module

ericvergnaud avatar Apr 03 '23 06:04 ericvergnaud

I filed an issue with TS asking how a package should be configured so that it can be consumed by a project using nodenext...hopefully they'll have a good answer, because I don't see any clear guidance in the docs.

jedwards1211 avatar Apr 03 '23 15:04 jedwards1211

Okay I think the .d.ts should have explicit file extensions in import/export from declarations. I enabled "traceResolution": true and saw this in the tsc output:

======== Resolving module './CharStreams' from '/Users/andy/antlr/node_modules/antlr4/src/antlr4/index.d.ts'. ========
Explicitly specified module resolution kind: 'NodeNext'.
Resolving in ESM mode with conditions 'import', 'types', 'node'.
Loading module as file / folder, candidate module location '/Users/andy/antlr/node_modules/antlr4/src/antlr4/CharStreams', target file types: TypeScript, JavaScript, Declaration.
Directory '/Users/andy/antlr/node_modules/antlr4/src/antlr4/CharStreams' does not exist, skipping all lookups in it.
======== Module name './CharStreams' was not resolved. ========

jedwards1211 avatar Apr 03 '23 16:04 jedwards1211

Oh and also if tsc --init hadn't set "skipLibCheck": true I would have seen this error:

node_modules/antlr4/src/antlr4/index.d.ts:19:15 - error TS2834: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

19 export * from './error';
                 ~~~~~~~~~

jedwards1211 avatar Apr 03 '23 16:04 jedwards1211

I had a ton of issues with importing Antlr, as it's published as an ESM module. For Mocha tests, it failed entirely, including with any options that supposedly support ESM. I finally had to switch to Jest, and transpile the published package before it was consumable. Publishing with "type": "module" breaks a lot of tools in the Node ecosystem.

matthew-dean avatar May 04 '23 01:05 matthew-dean

Hi, see PR #4217.

I use Mocha with ts-node, and it works perfectly: --loader=ts-node/esm --require ts-node/register

ericvergnaud avatar May 04 '23 09:05 ericvergnaud

@ericvergnaud Yeah, I tried that, I tried a billion different ways, but it sounds like 4.12.1 will solve it regardless.

matthew-dean avatar May 04 '23 14:05 matthew-dean

Can you show me an example about how did you managed to run jest with ANTLR4, @matthew-dean ?

danielzhe avatar Aug 30 '23 21:08 danielzhe

@danielzhe The Antlr4 distribution of TypeScript is fundamentally broken. The honest answer is that I went back to using Chevrotain.

matthew-dean avatar Sep 06 '23 18:09 matthew-dean

From my experiments with this the problem is here:

https://github.com/antlr/antlr4/blob/dev/runtime/JavaScript/src/antlr4/index.d.ts

This code

export * from "./InputStream";
export * from "./FileStream";
export * from "./CharStream";
export * from "./CharStreams";
export * from "./TokenStream";
export * from "./BufferedTokenStream";
export * from "./CommonToken";
export * from "./CommonTokenStream";
export * from "./Recognizer";
export * from "./Lexer";
export * from "./Parser";
export * from './Token';
export * from "./atn";
export * from "./dfa";
export * from "./context";
export * from './misc';
export * from './tree';
export * from './state';
export * from './error';
export * from './utils';
export * from './TokenStreamRewriter';

needs to be changed to full explicit file paths to adhere to the new rules for packages that have {"type": "module"} set in package.json as the antlr4 package has. So it should be:

export * from "./InputStream.js";
export * from "./FileStream.js";
export * from "./CharStream.js";
export * from "./CharStreams.js";
export * from "./TokenStream.js";
export * from "./BufferedTokenStream.js";
export * from "./CommonToken.js";
export * from "./CommonTokenStream.js";
export * from "./Recognizer.js";
export * from "./Lexer.js";
export * from "./Parser.js";
export * from './Token.js';
export * from "./atn/index.js";
export * from "./dfa/index.js";
export * from "./context/index.js";
export * from './misc/index.js';
export * from './tree/index.js';
export * from './state/index.js';
export * from './error/index.js';
export * from './utils/index.js';
export * from './TokenStreamRewriter.js';

And then for each of the above that imports index.js the same changes need to happen in those index.d.ts files.

Note that, typescript should point to .js rather than .ts files. This is correct and there is a lot of discussion on this on the typescript repo.

jonaskello avatar Nov 09 '23 09:11 jonaskello

Actually there are many places where .js needs to be added. It seems the antlr4 package is written in javascript and then the typescript types are manually handled by hand-written .d.ts files? In that case I could do a PR to update them all. Would that be accepted?

jonaskello avatar Nov 09 '23 10:11 jonaskello

based on https://www.typescriptlang.org/docs/handbook/modules/theory.html, I stumbled on https://arethetypeswrong.github.io/?p=antlr4%404.13.1-patch-1 which seems to be a promising tool to analyze this issue.

I've a patch that satisfies the attw checker in all except the "bundler" categories and the looks plausible to me based on the theory from the docs... should I create a PR? (I'm not a ts expert, so I'm not sure if my approach is the most correct one)

seb314 avatar Feb 16 '24 10:02 seb314

tbh I suspect that it might be impossible to satisfy all scenarios given the mess created over time by ts vs js and esm vs common js. Basically, we need:

  • the ability to run tests locally (without packaging) using node and ts-node
  • the ability to package for node commonjs
  • the ability to package for node esm
  • the ability to package for browser esm I will only consider PRs that provide evidence (i.e. successful tests) that all the above work. Otherwise it's likely that fixing an issue for 1 scenario is actually breaking another scenario...

ericvergnaud avatar Feb 16 '24 11:02 ericvergnaud

If I have understood correctly the current approach is hand writing the JS and then putting the types on from the outside and trying to have this cover every possible target. I think this makes it really hard to handle.

I would recommend writing the code in typescript and then compiling multiple times for the different targets (using different tsconfig settings for module target commonjs, esm etc).

Languages that have a binary target often do cross compile for different CPU architectures (x86, x64, arm). You can think of the JS output from a typescript compile as output for different targets (commonjs, esm, browser).

You can then distribute multiple sets of files and have the package.json specify the location of each set. See these docs for how that works: https://nodejs.org/api/packages.html#conditional-exports

jonaskello avatar Feb 16 '24 13:02 jonaskello

We are already "compiling" (with webpack) for different targets, so not sure migrating to TS would improve things.

Re conditional exports, we have them in our current package.json, but they follow a different style than the style in the provided link. Comments are welcome.

But what would really help is a test infrastructure for trying out all these targets. If someone is willing to create that, it would be great, because then we can make decisions based on facts.

ericvergnaud avatar Feb 18 '24 13:02 ericvergnaud

We are already "compiling" (with webpack) for different targets, so not sure migrating to TS would improve things.

I can think of a couple of things here:

  • You write once in TS and then compile using tsc into JS and types. This is much easier than writing JS and types separately by hand for every case. I don't think webpack is very good at handling bundling of types.
  • The output artifacts (JS+types) are going to be consumed by tsc and the probability that output from tsc is consumable by tsc is higher than hand written types and webpack transforms.
  • The bundler space if moving fast and webpack is being replaced by newer tools such as vite, esbuild, turbopack. Meanwhile tsc is still the same tool maintained by the same company since 2012 - so much less tool churn.

I'm not saying it is not possible to handwrite the types separate from the JS and fiddle with a bundler (eg. webpack) to get it transformed to different targets. I'm just saying IMO it is a lot more work and tool churn to do it that way.

Btw, ESM is natively supported in node since version 12 so if there are no special reasons to support earlier versions of node it would be possible to drop the commonjs distribution.

jonaskello avatar Feb 18 '24 22:02 jonaskello

Thanks. I use the proposed approach in another project. But the JS runtime was written 12 years ago, long before ES6 (classes), ESM or TypeScript. There’s a lot of code out there relying on now legacy stuff, and we can’t break backwards compatibility.

ericvergnaud avatar Feb 18 '24 23:02 ericvergnaud

Not sure what you are referring to but using typescript would not break any combability with any JS runtimes. If we turn it around, what are the arguments to not simply write this package in typescript?

jonaskello avatar Feb 18 '24 23:02 jonaskello

The exports wouldn't be backwards compatible. It requires a very significant effort which introduces risk when we have a very stable runtime. Currently our efforts are geared towards antlr5 which will be TS only.

ericvergnaud avatar Feb 19 '24 05:02 ericvergnaud

The exports wouldn't be backwards compatible.

I don't follow, could you be more specific? Like the exports of X would not be compatbile with Y?

If you are thinking about the output of tsc in regards towards module systems, it can output for any system, it is just a compiler option. Typescript has been around for 14 years and AFAIK it can be compatible with anything within that timeframe or even before that.

Perhaps my comment above about native support for ESM in node 12+ was confusing, this comment is totally unrelated to the use of typescript.

jonaskello avatar Feb 19 '24 06:02 jonaskello

tbh I suspect that it might be impossible to satisfy all scenarios given the mess created over time by ts vs js and esm vs common js. Basically, we need:

* the ability to run tests locally (without packaging) using node and ts-node

* the ability to package for node commonjs

* the ability to package for node esm

* the ability to package for browser esm
  I will only consider PRs that provide evidence (i.e. successful tests) that all the above work. Otherwise it's likely that fixing an issue for 1 scenario is actually breaking another scenario...

@ericvergnaud do you have any pointers about how such tests should look like?

Currently I imagine something like: A set of small projects (one for esm vs commonjs, for node vs targeting browsers via webpack) that each depend on the js runtime, generate a parser and parse some string. We could use typescript in each of them and configure it to output for the corresponding target. That way the tsc compilation step of each test project would check that tsc can resolve the typings, and the subsequent invocationg of the parser should serve as a basic thest that the js works at runtime too. (I think we'd need different projects rather than just test cases within ./runtime/JavaScript, in order to make sure that the packaged version works correctly, not just the local sources).

(If there is some tool that can automatically perform these tests, without the need to explicitly setup the test projects, that would be nice of course.)

seb314 avatar Feb 23 '24 10:02 seb314

I agree that would be the way to test it. Have tsc consume each scenario and see if it works.

I would add that if tsc is used to create the output for a certain scenario the chance that tsc is able to successfully consume that scenario would be very high, even to the point where tests are not needed. However it is always nice to have tests anyway.

I'm sorry for pushing for tsc to produce the output, I'm just afraid that without allowing appropritate tooling to easily solve this issue it will require such an amount of manual work and bundler config fiddeling that it will not ever get solved.

jonaskello avatar Feb 23 '24 11:02 jonaskello

@jonaskello I also believe that it would in principle be preferable to have a single source of truth in .ts files. But 1) I believe the resulting js would not be 1:1 identical to the current hand-written js, so it might be hard to know that this refactoring doesn't break some important use case that works right now and 2) there are >100 .js files in the current runtime, so in any case the migration would be a considerable effort*

=> I think it is a more manageable first step to add some tests and then try to fix just the type declarations (mainly add the .js extensions for the imports). That change would help projects that depend on antlr and use typescript, and because we don't even touch the js files, there shouldn't be much risk of breaking any behavior at runtime.

If someone with more js/ts experience than me would like to recommend a better approach, go ahead :-)

  • not sure if there are tools that could automatically merge .js + .d.ts -> .ts, but even if those exist they might fail because the current .d.ts files don't seem to 100% accurately match the js files.

seb314 avatar Feb 23 '24 12:02 seb314

I just run into this problem in a Vite project. I read the whole thread and unless I am missing something, no big changes are needed for this to work.

The issue is only a configuration error. Some fields needs to be added to package.json and that's it. Also there are some extra files in the bundle (like src files, tsconfig, babel, webpack config, tests etc) so I guess it should be ok if they are removed too.

I will open a PR as soon as I can. But I am not sure how I should write tests for it.

KurtGokhan avatar Feb 23 '24 14:02 KurtGokhan

My bad, I just realized that my change fixes the issues with Bundler module resolution but not for NodeNext. File extensions are required after all.

KurtGokhan avatar Feb 23 '24 14:02 KurtGokhan

Sorry for flooding. But there was a simple solution after all. Created PR at #4540

In summary:

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.

KurtGokhan avatar Feb 23 '24 15:02 KurtGokhan

@seb Since typescript is a strict superset of javascript, the migration could be as easy as just renaming the files from .js to .ts. This way the output will also be the same as previous js at least for one set of compiler options. The typescript team put a lot of effort into keeping this kind of scenario possible. In addition other compiler options could be used to create different output that is transformed to commonjs etc.

I think not many types from the existing .d.ts files would need to be added to the .ts files since this package heavily uses classes to represent types and therefore will have nominal typing of classes automatically applied by tsc without any additional type annotations needed (as opposed to the structural typing offered by interfaces or type alias).

So migration to ts would mean exactly same js output but the .d.ts would be different since it would be automatically generated by tsc and therefore 100% correct instead of the current broken one.

jonaskello avatar Feb 24 '24 10:02 jonaskello

Currently I imagine something like: A set of small projects (one for esm vs commonjs, for node vs targeting browsers via webpack) that each depend on the js runtime, generate a parser and parse some string. We could use typescript in each of them and configure it to output for the corresponding target.

Yes we need exactly that. To be more accurate it would have to rely on the webpacked runtime. That would definitely help validate #4540

ericvergnaud avatar Feb 24 '24 11:02 ericvergnaud

@ericvergnaud the doc about community conditions simply says they have to come first. No mention is made about the possibility to nest them.

This simple patch fixes everything :

"exports": {
    ".": {
++    "types": "./src/antlr4/index.d.ts",
      "node": {
--      "types": "./src/antlr4/index.d.ts",
        "import": "./dist/antlr4.node.mjs",
        "require": "./dist/antlr4.node.cjs",
        "default": "./dist/antlr4.node.mjs"
      },
      "browser": {
--      "types": "./src/antlr4/index.d.ts",
        "import": "./dist/antlr4.web.mjs",
        "require": "./dist/antlr4.web.cjs",
        "default": "./dist/antlr4.web.mjs"
      }
    }
  }

JesusTheHun avatar Mar 01 '24 12:03 JesusTheHun