-
Notifications
You must be signed in to change notification settings - Fork 14
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
Suggestions on performance improving #53
Comments
I'm not sure how compiler options and dependency resolution affect organizing imports. It looks like they shouldn't really affect it, but still I'm somehow sure there are tricky edge cases I'm not aware of. What if we add an explicit |
I just tested the following code and it works fine for me. // cli.js:L45
/**
* @type {Record<string, {
* files: 'all' | SourceFile[]
* project: import('ts-morph').Project,
* detectNewLineKind: boolean,
* }>}
*/
const projects = {};
const defaultProject = new Project();
defaultProject.addSourceFilesAtPaths(filePaths);
projects.defaultProject = {
project: defaultProject,
files: defaultProject.getSourceFiles(),
};
// The whole `for` loop is commented out, for test purpose only
// for (const filePath of filePaths) {
// const tsConfigFilePath = tsconfig.findSync(path.dirname(filePath)); I agree with the Though I don't have a clear idea neither, how about |
|
Any updates? |
@afterwind-io @thorn0 The suggested options can be added to https://github.com/thorn0/organize-imports-cli/blob/master/cli.js#L74 & https://github.com/thorn0/organize-imports-cli/blob/master/cli.js#L103-L105. Is this correct? Happy to open a PR to have this shipped. |
It has been quite a while and if I recall correctly... https://github.com/thorn0/organize-imports-cli/blob/master/cli.js#L74 Yes. https://github.com/thorn0/organize-imports-cli/blob/master/cli.js#L103-L105 Not sure. Seems yes. Haven't tried manipulating this before. |
It's the opposite. The first link leads to a code path where tsconfig is used. We want to skip it when the new flag is specified. Please go ahead with the PR @glennreyes. |
Would love to have this feature added |
TL;DR: A little bit changes to the options of the
Project
constructor will greatly reduce the time consuming during the initializing of the new instance.In most daily cases, imports organizing only deal with files on a one-to-one basis. There is no need to know the whole detail of the project(s), nor the dependency relations of every single file. The only thing wo do need is the pure AST.
Currently, the default options of the
Project
constructor, which defined byts-morph
, is quite opposite to our ideal scenario:https://github.com/dsherret/ts-morph/blob/e769b7871982ca874684e20355d58f1153efdcda/packages/ts-morph/src/Project.ts#L11-L34
FYI, we use
organize-imports-cli
in a middle size project, managed bylerna
, with dozens of inline packages. That means, when commited files scatter acrose several packages, each package will produce an instance ofProject
, and each instance costs 3 seconds on average, becausets-morph
is trying to load all the files during initializing, though most of them are totally irrelevant.I have tried modify the code locally, and passed all the tests, except the
excluded test
, which is caused byaddFilesFromTsConfig
. So if you are willing to alter the options, there will be some more effort needed to deal with file resolving when included files defined in thetsconfig.json
are really all needed. But for almost all daily cases, it will just works fine.The text was updated successfully, but these errors were encountered: