tunnelvisionlabs / antlr4ts

Optimized TypeScript target for ANTLR 4
Other
624 stars 106 forks source link

Circular import dependencies #466

Open BurtHarris opened 4 years ago

BurtHarris commented 4 years ago

Doing some experimentation with Rollup.js, I find that it (among many other things) identifies and generates a warning for cycles in import dependencies. The code-base has quite a number of these, 14 if I recall. These are not errors, but point up some potential problems due to incomplete initialization of dependencies.

Some cycles can be resolved quite trivially, for example 3 cycles can be addressed by moving definition of constants MIN_CHAR_VALUE and MAX_CHAR_VALUE from Lexer.ts to somewhere like misc/Character.ts (which has no imports.) A number of the other cycles are similar, where some simple constants declared in a module with a high dependency count could be move to one with low dependencies to resolve.

But the most ugly cycles are indirect ones involving ATN.ts.

I'm going to stop exploring these right now, but they don't smell good.

BurtHarris commented 4 years ago

I think this message is generated by a circular import dependency problem:

Exception has occurred: TypeError
TypeError: Class extends value undefined is not a constructor or null
    at Object.<anonymous> (c:\code\antlr4ts\packages\tour\node_modules\antlr4ts\dist\tree\xpath\XPathLexer.js:9:30)
    at Module._compile (internal/modules/cjs/loader.js:1158:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1178:10)
    at Module.load (internal/modules/cjs/loader.js:1002:32)
    at Function.Module._load (internal/modules/cjs/loader.js:901:14)
    at Module.require (internal/modules/cjs/loader.js:1044:19)
    at require (internal/modules/cjs/helpers.js:77:18)
    at Object.<anonymous> (c:\code\antlr4ts\packages\tour\node_modules\antlr4ts\dist\tree\xpath\XPath.js:13:22)
    at Module._compile (internal/modules/cjs/loader.js:1158:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1178:10)
BurtHarris commented 4 years ago

Good article on this problem: How to fix nasty circular dependency issues once and for all in JavaScript & TypeScript.

The solution offered is called the internal module pattern, and in TypeScript/JavaScript it is implemented as follows:

The crux of this pattern is to introduce an index.js and internal.js file. The rules of the game are as follows:

  1. The internal.js module both imports and exports everything from every local module in the project
  2. Every other module in the project only imports from the internal.js file, and never directly from other files in the project.
  3. The index.js file is the main entry point and imports and exports everything from internal.js that you want to expose to the outside world.
BurtHarris commented 4 years ago

Complicated by generated imports using "src" in the path, which have to go.

IceMikl commented 3 years ago

Is there any progress in solving this issue? Did anybody find a workaround?

I had following output by circular dependency error: Error: Circular dependency: node_modules\assert\build\assert.js -> node_modules\assert\build\internal\errors.js -> node_modules\assert\build\assert.js Error: Circular dependency: node_modules\assert\build\assert.js -> node_modules\assert\build\internal\errors.js -> C:\...\node_modules\assert\build\assert.js?commonjs-proxy -> node_modules\assert\build\assert.js Error: Circular dependency: node_modules\antlr4ts\atn\ATN.js -> node_modules\antlr4ts\dfa\DFA.js -> node_modules\antlr4ts\atn\ATNConfigSet.js -> node_modules\antlr4ts\atn\ATN.js Error: Circular dependency: node_modules\antlr4ts\atn\PredictionContext.js -> node_modules\antlr4ts\atn\PredictionContextCache.js -> node_modules\antlr4ts\atn\PredictionContext.js Error: Circular dependency: node_modules\antlr4ts\atn\PredictionContext.js -> node_modules\antlr4ts\atn\PredictionContextCache.js -> C:\...\node_modules\antlr4ts\atn\PredictionContext.js?commonjs-pr oxy -> node_modules\antlr4ts\atn\PredictionContext.js Error: Circular dependency: node_modules\antlr4ts\atn\ATN.js -> node_modules\antlr4ts\dfa\DFA.js -> node_modules\antlr4ts\atn\ATNConfigSet.js -> C:\...\rest4fta-editor-mendixwidget\node_modules\antlr4ts\atn\ATN.js?commonjs-proxy -> node_modules\antlr4ts\atn\ATN.js .....

usstwxy commented 2 years ago

+1

got error when using Parser which is caused circular import in vite rollup

Exception has occurred: TypeError
TypeError: Class extends value undefined is not a constructor or null
adiun commented 2 years ago

Is there any workaround here @BurtHarris? We also hit the same issue that @usstwxy hit, using Vite rollup

yangjunhan commented 2 years ago

+1

During the building process, rollup.js reported: (!) Circular dependencies node_modules/antlr4ts/atn/ATN.js -> node_modules/antlr4ts/dfa/DFA.js -> node_modules/antlr4ts/atn/ATNConfigSet.js -> node_modules/antlr4ts/atn/ATN.js node_modules/antlr4ts/atn/ATN.js -> node_modules/antlr4ts/dfa/DFA.js -> node_modules/antlr4ts/atn/ATNConfigSet.js -> /Users/ziyang/WebstormProjects/flink-languageservice/node_modules/antlr4ts/atn/ATN.js?commonjs-proxy -> node_modules/antlr4ts/atn/ATN.js node_modules/antlr4ts/atn/PredictionContext.js -> node_modules/antlr4ts/atn/PredictionContextCache.js -> node_modules/antlr4ts/atn/PredictionContext.js ...and 23 more

which consequently result the TypeError: Class extends value undefined is not a constructor or null error if importing the dist package.

Luna-Klatzer commented 2 years ago

Are there any updates or plans to fix this? Since I am also having issues with simply importing antlr4ts using require() in a JS environment.

Here is an example on RunKit simply using the default antlr4ts package: https://runkit.com/embed/vewk6ve5pu4k

jimtendo commented 7 months ago

Hi, also run into this. Unfortunately, antlr4ts is a dependency of another package I'm using.

Has anyone found a workaround to this?

jimtendo commented 7 months ago

Hi, also run into this. Unfortunately, antlr4ts is a dependency of another package I'm using.

Has anyone found a workaround to this?

I have not found a solution to this for the package I am using (cashc).

However, in case it may work for others, there is an (older) version on NPM I managed to get running with Vite.

https://www.npmjs.com/package/@panda2134/antlr4ts-esm

I was then able to set Vite to alias antlr4ts to antlr4ts-esm like so:

viteConf.resolve.alias['antlr4ts'] = '@panda2134/antlr4ts-esm';

NOTE: This will not work for cashc - the contracts will not compile correctly (suspect version mismatch as culprit).

If anyone has found a workaround that might work for latest version, that'd be much appreciated.

jimtendo commented 7 months ago

Just an update in case anyone else is looking for some form of workaround:

The short of it is, I didn't find a good approach for this. I'm tied to Vite+Rollup for my build system.

However, as my usage was with the cashc library (CashScript Compiler for BCH), I ended building a wrapper package that bundles cashc + antlr4ts into a single output using Webpack. This appears to work fine.

https://www.npmjs.com/package/cashc-web

If you're in a dire situation, you can probably just copy that repo and adjust slightly to your needs.