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

typeCheck is too slow #76

Open
jbedard opened this issue May 12, 2017 · 38 comments
Open

typeCheck is too slow #76

jbedard opened this issue May 12, 2017 · 38 comments

Comments

@jbedard
Copy link

jbedard commented May 12, 2017

The typeCheck option makes builds extremely slow because the Program is recreated for each file.

gulp-tslint solves this by forcing the user to pass the Program in as part of the configuration. I think the same could be done here by replacing the typeCheck with a program option? Or maybe allow passing the Program instead of a boolean to the typeCheck? Otherwise it will have to be cached somehow across files.

@sonicoder86
Copy link
Collaborator

Is it realistic to have more than one loader linting inside a Webpack configuration?

@jbedard
Copy link
Author

jbedard commented May 12, 2017

Are you suggesting just caching the Program globally?

You can easily have multiple webpack configurations running at the same time, with different lint options etc.

@sonicoder86
Copy link
Collaborator

Maybe caching based on the webpack instance is ok.

@jbedard
Copy link
Author

jbedard commented May 12, 2017

If the webpack instance is recreated per webpack config that should work...

@garethflowers
Copy link

We're seeing this issue too. Simply enabling the typeCheck in our project has increased build time from 2 minutes to 40 minutes!!

Using tslint-loader 3.5.3, tslint 5.2.0, webpack 2.5.1.

@sanex3339
Copy link

same here

1 similar comment
@szebrowski
Copy link

same here

@simonvizzini
Copy link

We also have performance issues with tslint-loader. Linting is much slower compared to the tslint CLI, even without the typeCheck option (with typeCheck it's even slower).

Now I wanted to create my own tslint watcher and so I was playing around with the tslint API and chokidar. I found out during my testing that it's not a good idea to cache the Program instance, because then tslint will always report the same errors, even if they have been fixed afterwards. Creating the Program instance for every lint seems to be necessary to get correct results.

I never used gulp-tslint, but I took a look at the code and I have no idea why caching the Program instance works there... I'm a bit baffled by this.

@simonvizzini
Copy link

simonvizzini commented May 24, 2017

For reference, this is the example code I tested this with:

const linterConfigFile = path.resolve(watchDir, "tslint.json");
const tsConfigFile = path.resolve(watchDir, "tsconfig.json");

// Tried to store program instance here, but linter gives wrong results then.
// const tsProgram = Linter.createProgram(tsConfigFile);

const lintFile = (filePath) => {
    // necessary to create tsProgram for every lint...
    const tsProgram = Linter.createProgram(tsConfigFile);
    const linter = new Linter({ formatter: "codeFrame" }, tsProgram);

    const config = Configuration.findConfiguration(linterConfigFile, filePath).results;
    const fileContent = tsProgram.getSourceFile(filePath).getFullText();

    linter.lint(filePath, fileContent, config);
    console.log(linter.getResult().output);
};

chokidar.watch(watchDir, chokidarOptions)
    .on("change", (filePath) => {
        console.log("changed:", filePath);
        lintFile(path.resolve(watchDir, filePath));
    })
    .on("error", console.error.bind(console));

@sonicoder86
Copy link
Collaborator

@simonvizzini Can you confirm it is also on the pull request to address this issue?

@simonvizzini
Copy link

Ah, I'm sorry, I totally missed the pull request. This is strange, I'm sure @mtraynham has tested this, and I guess it worked for him. And if it works in gulp-tslint as well then why not here. Not really sure why it didn't work in my minimal sample. I guess this could be a case of "doesn't work on my machine" (work machine which runs Win10).

I'm at home now and I won't be able to investigate this further until monday, but I'll do a quick test later today on my linux machine at home.

@simonvizzini
Copy link

simonvizzini commented May 30, 2017

I haven't had much time to investigate this further but I found this interesting note in the gulp-tslint README regarding the typeCheck feature.

// NOTE: Ensure 'Linter.createProgram' is called inside the gulp task else the contents of the files will be cached
// if this tasks is called again (eg. as part of a 'watch' task).

This is exactly what I'm experiencing when I cache the program instance, I get back cached results. And if I understand gulp correctly then the gulp task is executed every time for every file change in watch mode, and thus creating a new Program instance every time. So as I see it it's not possible to cache the Program instance and so I don't think the related pull request will work at all. Or I'm still missing something...

@mtraynham
Copy link

mtraynham commented Jun 5, 2017

@simonvizzini Yeah, that does look like a problem with caching the Program object with only the loader. Although, I assume this is only a problem because you are using Dev Server or some variant of watch mode. That PR may be ok for simple builds.

Alternatively, adding a compiler plugin for this loader which supports events for run & watch-run, we could use those event hooks to recreate the Program object for a "run".

The awesome-typescript-loader does something similar, so it only compiles changed files.

@zuzusik
Copy link

zuzusik commented Jun 6, 2017

confirming the slowness - build time increased from 1.5 minutes to 15 minutes

@simonvizzini
Copy link

@mtraynham you are right that this would work for a single run webpack build, but webpacks --watch flag is an essential feature and should be supported by all loaders out of the box. But I guess your proposal would work.

Also, during my own tslint performance testing I found out that creating the Program object isn't that expensive (unless it is re-created for every single file of course, which only makes sense in a watch mode).

Here is a comparison between running tslint on our codebase directly and using tslint-loader:

webpack without tslint-loader (includes assets, less, etc): ~30 secs
webpack with tslint-loader and type checking enabled: ~80 secs
tslint CLI, with typechecking: ~6 secs

Using tslint-loader with type checking takes an additional 50 seconds for our webpack build to complete. But using tslint directly from the CLI instead takes only around 6 seconds.

So the performance issues people are reporting here aren't really tslint or typechecking related. As I see it the problem must be either in tslint-loader or webpack itself.

I wrote my own tslint-watcher based on the example I posted in a comment above. It works fine and is super fast. Though it would be best to look into tslint-loader/webpack and try to fix any problems there, I just don't have enough time to do that.

@zuzusik
Copy link

zuzusik commented Jun 6, 2017

Looks we need to teach tslint-loader 2 things:

  1. cache program (already done here: Ref #76: Pass program for performance gain #78)
  2. re-create program on watch event - as far as I understand this can be done with webpack compiler plugin

@juanpicado
Copy link
Contributor

I can confirm this issue here as well. webpack@2.7.0, typescript@2.4.1, ts-loader@2.2.1 My build times goes from 1m to 20m.

@Martinspire
Copy link

So any news on this?

@use-strict
Copy link
Contributor

It's basically unusable. Build time increases by a factor of 10. Sometimes it crashes as well.

@jony89
Copy link

jony89 commented Sep 5, 2017

no news regard this? any plans to fix this?

@akagr
Copy link

akagr commented Sep 8, 2017

This is not in any way a fix, but I found I can get by for now with following:

  1. Using tslint-loader with typechecking disabled in webpack
  2. Relying on my editor's typescript capabilities (spacemacs - includes type checking) for normal development
  3. Add a pre-push hook to project that runs a full on tslint with typecheck enabled.

Obviously, would love to see this issue get resolved.

@sanex3339
Copy link

I switched on https://www.npmjs.com/package/tslint-webpack-plugin

It much better than this loader.

@pelotom
Copy link

pelotom commented Sep 10, 2017

@sanex3339 tslint-webpack-plugin doesn't seem to have a way to enable type information, nor a way to specify a particular tslint.json.

@michal-filip
Copy link

It's probably just not explicitly documented in the plugin. See these configuration options:
https://github.com/palantir/tslint/blob/master/src/runner.ts#L41
https://github.com/palantir/tslint/blob/master/src/runner.ts#L106
Try passing those flags along with your files definition.

@pelotom
Copy link

pelotom commented Sep 11, 2017

@michal-filip thanks, that does indeed work!

@pelotom
Copy link

pelotom commented Sep 11, 2017

I'm now happily using tslint-webpack-plugin in dev, but because it doesn't report errors/warnings thru webpack and thus isn't able to fail the build, I still have to use tslint-loader in CI.

@Martinspire
Copy link

While it is a nice workaround, I think its still better to make sure this bug is fixed in this loader and the issue should also not be closed yet.

@keradus
Copy link

keradus commented Sep 29, 2017

any news here ?

idiotWu added a commit to idiotWu/smooth-scrollbar that referenced this issue Oct 7, 2017
@mxchange
Copy link

Still insanely slow

@jacobbogers
Copy link

jacobbogers commented Nov 12, 2017

this issue is 6 months old, any progress on this?

@pelotom
Copy link

pelotom commented Nov 12, 2017

There haven’t even been any commits to this repo in the last 6 months, it seems safe to say the project is unmaintained at this point.

@mtraynham
Copy link

mtraynham commented Nov 13, 2017

You can use #78, and follow #78 (comment) to fix this.

Although, I wrote that PR (shameless plug), I'm not actually using this loader any more. I have switched to fork-ts-checker-webpack-plugin (see my last #78 (comment)). ts-loader has been refactored to handle thread-loader and has offloaded type checking to the fork-ts plugin, which runs in a completely separate process from Webpack and doesn't block the build. fork-ts handles both typecheck and tslint.

My builds were taking ~35 seconds. With ts-loader, thread-loader and fork-ts, it takes about ~11 seconds now.

@tplk
Copy link

tplk commented Nov 22, 2017

Looks like type check is deprecated: palantir/tslint#3322
@wbuchwalter, @BlackSonic do we still need this issue?

@szebrowski
Copy link

Yes, we need. Instead of having to use --type-check --project, now we only need to use --project. There are no other changes. This is strictly cosmetic.

@grzegorzjudas
Copy link

So, is there an option that I can use to pass the "project" in webpack config? Adding tsConfigFile still gets me all the warnings about type information being required.

@wbuchwalter
Copy link
Owner

Hey everyone, sorry this is issue is not fixed, we seemed to be lacking contributors in this project :)
While I am the original author of this project, I have stopped using TypeScript 2 or 3 years ago, so I am definitly not the best person to fix this.
@BlackSonic is probably busy with some other things as well.

If you know of another TS linter that already works better than this one, please let me know, I can mark this project as unmaintained and link to the other project.

If there is no other suitable linter and some of you are willing to be maintainer of this project, then please see : #40.

@nfm nfm mentioned this issue Dec 20, 2018
@spiltcoffee
Copy link

@wbuchwalter

tslint is currently in the process of being deprecated in favour of eslint and a project called typescript-eslint: palantir/tslint#4534

Perhaps the best way forward is to direct users of tslint-loader towards investigating eslint-loader instead?

@wbuchwalter
Copy link
Owner

@spiltcoffee Sounds good. Will add a note in the README.

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

No branches or pull requests