dependents / node-dependency-tree

Get the dependency tree of a module
MIT License
707 stars 84 forks source link

feat: pre-compile the typescript config #114

Closed FauxFaux closed 4 years ago

FauxFaux commented 4 years ago

This compilation step, currently done inside filing-cabinet, causes a significant slowdown for large trees. It happens that the existing code already supports passing an object in, so we can do that.

Config loading mostly copied from: https://github.com/dependents/node-filing-cabinet/blob/74185e06edbbcbb3062265946adbada9b5584999/index.js#L199-L201

In madge's usecase, this offers a ~10x speedup. For bin/cli.js, on one or two random files in the same project, I see >3x speedup.

c.f. https://github.com/pahen/madge/pull/237

mrjoelkemp commented 4 years ago

Thanks for contributing @FauxFaux! I just wrote a test for this, merged, and released it in a new patch version.

Thanks again!

LFDM commented 4 years ago

Warming up a very old topic...

This unfortunately breaks TS config files, which use the extends feature.

Return .raw returns really just the raw json representation of a tsconfig - as a result the compiler options used withing filing-cabinet will be incomplete, therefore leading to potentially wrong/missed module resolutions.

IMO this is already a problem within filing-cabinet, which either accepts a string path or a json object of the tsconfig. filing-cabinet actually goes on an either parses the string path OR the json object to compilerOptions. As it's not trivially possible to return a properly merged json representation of an extended tsconfig, the best way forward IMO might be to allow filing-cabinet to receive the compilerOptions directly. In addition to leading to the correct behavior, this would probably speed up things even further as well.

This might unfortunately only be possible with a breaking change, or by passing a new property "tsCompilerOptions" in addition to tsConfig.

@mrjoelkemp what do you think? I will go ahead and for both filing-cabinet and dependency-tree and try this out, as i need it for my own purposes anyway.

LFDM commented 4 years ago

Implemented here