Skip to content

HappyPack with ts-loader. #336

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

Closed
aindlq opened this issue Oct 22, 2016 · 28 comments
Closed

HappyPack with ts-loader. #336

aindlq opened this issue Oct 22, 2016 · 28 comments

Comments

@aindlq
Copy link

aindlq commented Oct 22, 2016

I'm trying to use HappyPack with ts-loader, and it seems to be working fine in transpileOnly mode if I remove lines:

https://github.com/TypeStrong/ts-loader/blob/v0.9.5/index.ts#L346
https://github.com/TypeStrong/ts-loader/blob/v0.9.5/index.ts#L718
https://github.com/TypeStrong/ts-loader/blob/v0.9.5/index.ts#L763

For me it is fine to remove L346 and L718 because I have tsc running in the background in typecheck mode. I wonder what are the implication of L763.

Also do you think it is possible to do some change to make it work out of the box without local patching of ts-loader.

see also amireh/happypack#33

@stefan-leye
Copy link

+1

@darthtrevino
Copy link

@aindlq Those lines don't look valid anymore. You may get more traction if you submit a PR

@johnnyreilly
Copy link
Member

Sorry yes - we've had a refactor. Those line numbers don't work anymore. Do you want to share more about your setup? It sounds kind of custom to have ts-loader and tsc running side by side...

@aindlq
Copy link
Author

aindlq commented Nov 2, 2016

I've updated links to lines that I changed.

The idea is to run ts-loader in transpile only mode with happypack in development, because it really helps to reduce build time on large projects.
And do typechecking outside of webpack, because anyway many developers already using some kind of IDE which is based on tsserver (Emacs, Atom, VSCode, etc.) so there is no need to run another type-checking process, especially when it takes >1GB of RAM and 10 seconds to typecheck our project.

In CI we do typechking with ts-loader and without happypack, because we want to have clean builds without any caching.

So far this workflow works quite good and what is more important it is quite fast with these 3 changes that I've mentioned, see also https://github.com/aindlq/ts-loader/commit/8d5badbdbb9e13838e321405b68c252fc484f9c9#diff-ed009b6b86b017532ef0489c77de5100

For us total webpack build went from 18 to 16 seconds with cold start, and down to 14 seconds with hot start. I would say 4 seconds worth the effort.
Here is some statistics from tsc for my project:

Files: 374
Lines: 126590
Nodes: 499538
Identifiers: 171293
Symbols: 570889
Types: 196220

@johnnyreilly
Copy link
Member

The push array changes are about registering errors with WebPack. I could see a scenario where we'd disable that with a flag. The last line I'm not so sure about though.

@johnnyreilly
Copy link
Member

Looking at the comment makes me think deactivating this could be a bad idea:

// Make sure webpack is aware that even though the emitted JavaScript may be the same as
    // a previously cached version the TypeScript may be different and therefore should be
    // treated as new
    this._module.meta.tsLoaderFileVersion = file.version;

@aindlq
Copy link
Author

aindlq commented Nov 3, 2016

That is exactly why I haven't submitted PR yet, because even so it works for me I'm not quite sure that it is a right change. So far I'm quite happy with my fork, but was wondering maybe there is a better way to do the same. Also I could imagine that similar setup can be interesting for other people as well.

@darthtrevino
Copy link

A PR doesn't necessarily have to be the ultimate solution; I've seen a lot
of people using them as a starting point for discussion and iteration

On Thu, Nov 3, 2016 at 2:00 PM, Artem Kozlov [email protected]
wrote:

That is exactly why I haven't submitted PR yet, because even so it works
for me I'm not quite sure that it is a right change. So far I'm quite happy
with my fork, but was wondering maybe there is a better way to do the same.
Also I could imagine that similar setup can be interesting for other people
as well.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#336 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAG7iPnBi6-qTbQkMtTBqASRvMFXEQa3ks5q6kuDgaJpZM4Kd_IR
.

@johnnyreilly
Copy link
Member

@darthtrevino is not wrong. BTW have you considered taking a look at awesome-ts-loader? I believe this may be actually catering for your particular setup already. Worth trying out. In the longer term we hope to join forces but in the short term you might find that this already suits your needs better than ts-loader.

@amireh
Copy link

amireh commented Nov 7, 2016

FWIW, I can gladly add support to managing loader._module.errors via HappyPack but the loader needs to adjust to use an unofficial API (e.g. duck-typing), which is why I've kept back from introducing such a change. The thing is, it can't be synchronous, so statements such as:

pushArray(loader._module.errors, ...)

Need to turn into something like this:

loaderInstance.registerModuleErrors(..., function(err) { })

Or just ignore the callback if you don't need to wait on it. Would this help?

(The same applies to the module "meta" hash, we can provide accessors.)

@abergs
Copy link
Contributor

abergs commented Feb 20, 2017

Any progress in this? :) We're running awesome-typescript-loader only for transpiling currently and we're looking to get Happypack in our build chain now when running webpack2.

@trusktr
Copy link

trusktr commented Mar 29, 2017

Please please pleeeeeeease, I am looking forward to super fast builds with TypeScript + HappyPack.

@johnnyreilly
Copy link
Member

See conversation in linked issue

@johnnyreilly
Copy link
Member

johnnyreilly commented May 21, 2017

Okay - thank to @aindlq we've added support for happypack. Things to note:

I'll look to ship this with ts-loader 2.1.0.

@johnnyreilly
Copy link
Member

ts-loader 2.1.0 has 🚢

@trusktr
Copy link

trusktr commented May 21, 2017

@johnnyreilly Maybe ts-loader can perform a separate type check first (no transpile), and then do the transpile? Maybe to do this there can be an option passed to ts-loader that does checking only. So basically ts-liader would be listed twice, one time as a direct loader with only type checking, and the other time as a loader used by happypack. The direct ts-loader would be listed last so that it runs first.

@trusktr
Copy link

trusktr commented May 21, 2017

F.e. something along the lines of

module: {
    use: [
        { test: /\.tsx?$/, loader: 'happypack/loader' },
        { test: /\.tsx?$/, loader: 'ts-loader?checkOnly' }
    ]
},
plugins: [
    new HappyPack({
        loaders: [ 'ts-loader' ]
    })
]

Maybe that's not exactly right, but you see what I mean, ts-loader used twice, one in check mode and the other in transpile mode with happypack.

Maybe the checkOnly mode can also be ran in happypack.

@johnnyreilly
Copy link
Member

@trusktr I'd advise plugging in the https://github.com/Realytics/fork-ts-checker-webpack-plugin as well for type checking when combined with happypack. I've given it a very quick test and it seems OK.

@johnnyreilly
Copy link
Member

Now tested using fork-ts-checker for typechecking. I've added a (somewhat complex) example setup here: https://github.com/TypeStrong/ts-loader/tree/master/examples/react-babel-karma-gulp-happypack

If someone wants to supply a simpler example this will be gladly received 😄

@johnnyreilly
Copy link
Member

@trusktr
Copy link

trusktr commented Jun 3, 2017

I'm not seeing any gains, although compilation is successful. For example, if I change my config from

     module: {
         rules: [ // note, loaders run last to first.
             {
                 test: /\.(ts|tsx)?$/,
                 use: [
                     'ts-loader',
                     //'happypack/loader?id=ts',
                     {
                         loader: 'tslint-loader',
                         options: {
                             formatter: 'codeFrame',
                             enforce: 'pre',
                             emitErrors: true,
                             typeCheck: true,
                             fix: false,
                         }
                     }
                 ],
                 exclude: /node_modules/
             },
         ],
     },
     plugins: [
         //new HappyPack({
             //id: 'ts',
             //threads: 4,
             //loaders: [
                 //{
                     //path: 'ts-loader',
                     //query: { happyPackMode: true }
                 //}
             //]
         //}),
     ],

to

    module: {
        rules: [ // note, loaders run last to first.
            {
                test: /\.(ts|tsx)?$/,
                use: [
                    //'ts-loader',
                    'happypack/loader?id=ts',
                    {
                        loader: 'tslint-loader',
                        options: {
                            formatter: 'codeFrame',
                            enforce: 'pre',
                            emitErrors: true,
                            typeCheck: true,
                            fix: false,
                        }
                    }
                ],
                exclude: /node_modules/
            },
        ],
    },
    plugins: [
        new HappyPack({
            id: 'ts',
            threads: 4,
            loaders: [
                {
                    path: 'ts-loader',
                    query: { happyPackMode: true }
                }
            ]
        }),
    ],

I still get the same build time (about a minute with either method).

What could make mine take the same amount of time either way? I'm in a VM, if that matters.

@trusktr
Copy link

trusktr commented Jun 3, 2017

That's without adding fork-ts-checker-webpack-plugin, so I suppose it'll start taking longer with that than raw ts-loader. If I remove tslint-loader, then it cuts the time in half for both methods, but they're still both the same at 30 seconds.

@johnnyreilly
Copy link
Member

I'm afraid I don't know what could be the problem with your build. However it looks like other people are seeing gains, could be worth asking in the comment thread here: amireh/happypack#33

@trusktr
Copy link

trusktr commented Jun 3, 2017

@johnnyreilly I haven't added fork-ts-checker plugin yet, but I still see type error emitted to console when using HappyPack and ts-loader in happyPackMode, and the build works. Is that expected?

@trusktr
Copy link

trusktr commented Jun 3, 2017

@johnnyreilly I'm using 2.1.0 as far as I know. That's what package.json and npm-shrinkwrap say, but as of NPM v5, package.json files no longer seem to be stored in node_modules, only code, so I can't see what version package.json says (f.e. with NPM v4 I could inspect package.json files in node_modules). hmmm.

@trusktr
Copy link

trusktr commented Jun 3, 2017

Hmm, maybe it was tslint-loader output? I can't seem to reproduce any more... I'll be back if it happens again.

@trusktr
Copy link

trusktr commented Jun 3, 2017

Although I can no longer reproduce, the errors at the time it happened were like the following:

ERROR in ./src/actions/AddFirstSplineAction.ts
(11,14): error TS2339: Property 'gameObject' does not exist on type 'AddFirstSplineAction'.

Are those ts-loader format?

@johnnyreilly
Copy link
Member

Maybe npm update to be sure. I don't think any errors should be reported in happypack mode

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

7 participants