Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Circular import dependencies #466

Open
BurtHarris opened this issue Apr 20, 2020 · 11 comments · May be fixed by #454
Open

Circular import dependencies #466

BurtHarris opened this issue Apr 20, 2020 · 11 comments · May be fixed by #454

Comments

@BurtHarris
Copy link
Collaborator

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
Copy link
Collaborator Author

BurtHarris commented Apr 23, 2020

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 BurtHarris self-assigned this Apr 23, 2020
@BurtHarris BurtHarris pinned this issue Apr 23, 2020
@BurtHarris
Copy link
Collaborator Author

BurtHarris commented Apr 23, 2020

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
Copy link
Collaborator Author

BurtHarris commented Apr 23, 2020

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

@BurtHarris BurtHarris linked a pull request Apr 26, 2020 that will close this issue
@BurtHarris BurtHarris linked a pull request Apr 26, 2020 that will close this issue
@IceMikl
Copy link

IceMikl commented Jul 5, 2021

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
Copy link

usstwxy commented Nov 6, 2021

+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
Copy link

adiun commented Jan 12, 2022

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

@yangjunhan
Copy link

+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
Copy link

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
Copy link

jimtendo commented Dec 5, 2023

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

Has anyone found a workaround to this?

@jimtendo
Copy link

jimtendo commented Dec 5, 2023

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
Copy link

jimtendo commented Dec 5, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants