tslint-loader icon indicating copy to clipboard operation
tslint-loader copied to clipboard

Ref #76: Pass program for performance gain

Open mtraynham opened this issue 7 years ago • 21 comments

As suggested in #76, we can pass the program so it's only created once per build. Alternatively, you could have the loader generate just once, instead of having the user pass it. But, I use both gulp-tslint and tslint-loader, so the consistency there is nice.

mtraynham avatar May 19 '17 01:05 mtraynham

@mtraynham can we restrict it only to typeCheck? I mean only use the provided program if typeCheck is true. I would stay with one switch for one feature.

sonicoder86 avatar May 19 '17 07:05 sonicoder86

~~Just so I understand, remove the program option and have the tslint-loader generate it (only once) instead?~~

Edit: Sorry, still early here. You want typeCheck to be a requirement, even if they pass program, got it.

mtraynham avatar May 19 '17 11:05 mtraynham

Alrighty, rebased. When users provide typeCheck: true, they can also provide the program option, or it will be resolved for them, created only once, and cached back to the options object for reuse.

mtraynham avatar May 19 '17 15:05 mtraynham

@mtraynham seems good, added one review

sonicoder86 avatar May 20 '17 06:05 sonicoder86

Actually, my mistake @blacksonic . This is still creating the program object every file... It looks like the options object is recreated every time a new file is linted. We'll have to store the program on the webpack instance.

mtraynham avatar May 22 '17 16:05 mtraynham

@blacksonic I think you may have annotated an old commit. That options object is getting recreated every time a new module runs through the loader (via these lines). We can't store the program on the options object because it get's discarded. Instead, I decided to store it on the webpackInstance.options, which is the same for the life of the process, and represents the original passed in options. It is stored as tsconfigProgram.

mtraynham avatar May 25 '17 03:05 mtraynham

Can you merge this PR please? It really gives an enormous perfomance boost.

disabled - 9389ms
enabled && typeCheck: false - 12414ms
enabled && typeCheck: true - 75928ms

https://github.com/wbuchwalter/tslint-loader/pull/78
enabled && typeCheck: true - 14189ms

tplk avatar Jun 05 '17 14:06 tplk

I just checked it locally - works as intended only for the first time - in watch mode shows exactly the same, "cached" errors with every incremental builds.

This should be fixed and also we should add a test to ensure it is capable to work correctly in watch mode?

zuzusik avatar Jun 06 '17 09:06 zuzusik

Any idea when this is going to be resolved?

tplk avatar Jun 26 '17 11:06 tplk

nudge

This is a major inefficiency and it would be great to see this PR merged!

pelotom avatar Sep 10 '17 22:09 pelotom

Guys, any news?

mxl avatar Sep 17 '17 22:09 mxl

The PR is not a full solution, because the first result gets cached

sonicoder86 avatar Sep 18 '17 07:09 sonicoder86

Rebased and corrected. When using Webpack watch mode, use the new TslintPlugin which clears the TypeScript program after each watch run.

var TslintPlugin = require('tslint-loader').TslintPlugin;

module.exports = {
    plugins: [
        new TslintPlugin()
    ],
    module: {
        rules: [
            {
                test: /\.ts$/,
                enforce: 'pre',
                loader: 'tslint-loader',
                options: { typeCheck: true, /* Loader options go here */ }
            }
        ]
    }
}

mtraynham avatar Sep 18 '17 17:09 mtraynham

Bump @blacksonic - could you please take a look?

JoshuaKGoldberg avatar Oct 20 '17 16:10 JoshuaKGoldberg

Just giving anyone a heads up. I wrote this pull, but I'm no longer using tslint-loader.

I have moved to fork-ts-checker-webpack-plugin. It runs TypeScript typechecking and optionally tslint in a secondary node process that does not affect the Webpack build process.

ts-loader now recommends this setup. Setting the happyPack flag (derived from also using the happypack loader) it performs transpilation only (transpileOnly), while fork-ts-checker is performing the typechecking & linting. That and the addition of cache-loader & thread-loader (which is inter-changable with happypack), it has now immensely improved performance over the awesome-typescript-loader.

Example of the suggested setup

mtraynham avatar Oct 25 '17 18:10 mtraynham

@mtraynham fork-ts-checker plugin runs async. Even with async set to false, there is no way to make the bundle not emit on error. I am using tslint-loader inside happypack with tslint-loader and it actually works faster for me than running fork-ts-checker separately. Besides that, avoid emitting on error works fine. Would you reconsider giving a try running tslint-loader inside happypack with your changes? I do believe this is a better alternative but thank you in any case.

eddyw avatar Nov 21 '17 00:11 eddyw

@eddyw

Even with async set to false, there is no way to make the bundle not emit on error.

Does this not work?

https://webpack.js.org/plugins/no-emit-on-errors-plugin/

pelotom avatar Nov 21 '17 01:11 pelotom

@pelotom no. I'm using the plugin but my bundle is still emitted.

That's the reason why I switched over to try this loader.

eddyw avatar Nov 21 '17 01:11 eddyw

This change would definitely be a nice-to-have. I noticed the performance difference as well and couldn't get any of the other TS Lint with webpack plugins to work the way I needed.

Skeeterdrums avatar Dec 06 '17 16:12 Skeeterdrums

Using TSLint Language Service in VSCode, I figure why ts-loader (or other TS loader) doesn't include the plugin in the Compiler. That would mean that, in a way, tslint would run together within the compiler instead of using a separate loader and passing all result all over again from one loader to another.

Of course, by default, TS plugins get ignored during compilation. However, there is a workaround to make them work as part of the compiler. Advantage of this? Messages of tslint are part of the compiler diagnostics and tslint is run at the same time while the file is being transpiled (and type checking as well).

The problem with ts-loader is that Type Checking doesn't work with HappyPack or thread-loader which instead increases build time. So I started to work on an experiment and finally decided to write a TS loader, with those ideas in mind, that works with HappyPack/thread-loader (including Type Checking) and runs tslint within the Compiler as a TS Language Service Plugin... https://github.com/eddyw/owlpkg-typescript-loader

It works in my projects. However, it may still need some work. Part of the ideas where also taken from this thread and others.

eddyw avatar Dec 08 '17 08:12 eddyw

@eddyw thanks for the info, I'll take a look at it. The primary reason I want TS Linting during compilation is for Checkstyle reporting as part of the automated build process. In most other scenarios, you're right: it doesn't make sense to couple these two things.

Skeeterdrums avatar Dec 08 '17 13:12 Skeeterdrums