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

Ref #76: Pass program for performance gain #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Ref #76: Pass program for performance gain #78

wants to merge 1 commit into from

Conversation

mtraynham
Copy link

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.

@sonicoder86
Copy link
Collaborator

@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.

@mtraynham
Copy link
Author

mtraynham commented May 19, 2017

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

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.

@sonicoder86
Copy link
Collaborator

@mtraynham seems good, added one review

@mtraynham
Copy link
Author

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.

Copy link
Collaborator

@sonicoder86 sonicoder86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check that the error remains or not if the program is cached? It is mentioned in the corresponding ticket.

index.js Outdated
program = Lint.Linter.createProgram(tsconfigPath);
if (!options.program) {
var tsconfigPath = resolveFile(options.tsConfigFile);
options.program = Lint.Linter.createProgram(tsconfigPath);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just assign it to program, it is not desired to modify an input object.

@mtraynham
Copy link
Author

@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.

@tplk
Copy link

tplk commented Jun 5, 2017

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

@zuzusik
Copy link

zuzusik commented Jun 6, 2017

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 zuzusik mentioned this pull request Jun 6, 2017
@tplk
Copy link

tplk commented Jun 26, 2017

Any idea when this is going to be resolved?

index.js Outdated
@@ -62,8 +62,15 @@ function lint(webpackInstance, input, options) {

var program;
if (options.typeCheck) {
var tsconfigPath = resolveFile(options.tsConfigFile);
program = Lint.Linter.createProgram(tsconfigPath);
if (!webpackInstance.options.tslintProgram) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compiler.options is deprecated.

@pelotom
Copy link

pelotom commented Sep 10, 2017

nudge

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

@mxl
Copy link

mxl commented Sep 17, 2017

Guys, any news?

@sonicoder86
Copy link
Collaborator

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

@mtraynham
Copy link
Author

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 */ }
            }
        ]
    }
}

@JoshuaKGoldberg
Copy link

Bump @BlackSonic - could you please take a look?

@mtraynham
Copy link
Author

mtraynham commented Oct 25, 2017

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

@eddyw
Copy link

eddyw commented Nov 21, 2017

@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.

@pelotom
Copy link

pelotom commented Nov 21, 2017

@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/

@eddyw
Copy link

eddyw commented Nov 21, 2017

@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.

@Skeeterdrums
Copy link

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.

@eddyw
Copy link

eddyw commented Dec 8, 2017

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.

@Skeeterdrums
Copy link

@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.

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

Successfully merging this pull request may close these issues.

10 participants