Skip to content
This repository has been archived by the owner on Jan 26, 2019. It is now read-only.

Migrate ts-loader to awesome-typescript-loader #74

Closed
wants to merge 15 commits into from
Closed

Migrate ts-loader to awesome-typescript-loader #74

wants to merge 15 commits into from

Conversation

Havret
Copy link

@Havret Havret commented May 25, 2017

Resolves #60.

I think it should do the trick, but please check if I didn't miss something. I tried this on my own project and it seems to work just fine.

@Havret
Copy link
Author

Havret commented May 25, 2017

Nah it seems it will be harder than I expected. Any ideas why this is failing?

@bryanmig
Copy link

I think one of the big wins using at-loader is its integration with Babel which is not part of this commit. Do you think that is something that should be added? Maybe it can be opt-in?

@Havret
Copy link
Author

Havret commented May 25, 2017

Precisely. It should, but I haven't figured out how to do it yet. Any suggestions?

@bryanmig
Copy link

This is extracted from my previously ejected CRA (pre-webpack2)

      {
        test: /\.tsx?$/,
        include: [paths.appSrc],
        loader: "awesome-typescript-loader",
        query: {
          useBabel: true
        }
      },

I basically just copy and pasted from their docs. Havent tried with latest CRA and Webpack2 but imagine its not much different.

Copy link

@bryanmig bryanmig left a comment

Choose a reason for hiding this comment

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

Nice!

@Havret
Copy link
Author

Havret commented May 25, 2017

[at-loader] Checking finished with 1116 errors
Error in bail mode: [at-loader] ./node_modules/@types/node/index.d.ts:26:11 
    TS2451: Cannot redeclare block-scoped variable 'Error'.

I am pretty much shooting in the dark so any help with this would be most welcome.

@bryanmig
Copy link

I see this is still failing. I will try to check out your branch if I have a chance today.

Copy link

@KatSick KatSick left a comment

Choose a reason for hiding this comment

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

What is the benefit of awesome-typescript-loader ?

@Havret
Copy link
Author

Havret commented May 27, 2017

Read their readme. I think it's worth to try to fight it through.

@Havret
Copy link
Author

Havret commented May 27, 2017

I made some progress but it is still failing with error:

Error in bail mode: [at-loader] TS6059: File '/tmp/tmp.yqU76FJR5i/test-app/config/env.js' is not under 'rootDir' '/tmp/tmp.yqU76FJR5i/test-app/src'. 'rootDir' is expected to contain all source files.

@bryanmig
Copy link

@Havret what happens if you remove rootDir from tsconfig entirely? I had a similar problem in a non CRA project and simply removed that and then it worked as expected.

@Havret
Copy link
Author

Havret commented May 28, 2017

@bryanmig Yep, it did the trick. :) Thank you so much!
@wmonk Could you please verify if everything is ok with this PR now?

@wmonk
Copy link
Owner

wmonk commented May 31, 2017

I'm not totally sure that removing the rootDir is the correct way to go for this, @DanielRosenwasser could you chime in on this?

@plievone
Copy link

plievone commented Jun 5, 2017

Hi, if you proceed with this, please measure some numbers (also with webpack --watch). Usually ts-loader is pretty fast when watching, and there's info how to speed it up (https://github.com/TypeStrong/ts-loader#faster-incremental-builds) and cautions about awesome-typescript-loader potentially slow watching speed (https://github.com/Realytics/fork-ts-checker-webpack-plugin#motivation). Haven't used those personally, though, as I usually just use ts-loader with transpileOnly: true and run a separate background task in vscode with tsc full compilation, so $tsc-watch problemMatcher highlights all errors across the project in the editor on save, not only the client-related ones, and webpack stays snappy.

@mattleff
Copy link
Contributor

mattleff commented Jun 8, 2017

@Havret @wmonk

Error in bail mode: [at-loader] TS6059: File '/tmp/tmp.yqU76FJR5i/test-app/config/env.js' is not under 'rootDir' '/tmp/tmp.yqU76FJR5i/test-app/src'. 'rootDir' is expected to contain all source files.

You should be able to solve that error by adding config to the exclude key (see here) in your tsconfig.json. That will work with the rootDir present. rootDir is necessary for cases where there are multiple Typescript projects in the same tree (such as a submodule).

@wmonk wmonk added the question label Jun 13, 2017
@mattleff
Copy link
Contributor

@wmonk @Havret I can confirm that adding config works fine. You can check out my diff here. With this change I am able to use babel-plugin-relay. 🎉

@Havret
Copy link
Author

Havret commented Jun 14, 2017

@mattleff all tests passed, after change you suggested.
@wmonk could you take a look at this?

@zinserjan
Copy link
Contributor

Is awesome-typescript-loader really the way to go?

I just read the notes about the poor performance for incremental builds with atl at fork-ts-checker-webpack-plugin and this makes me very nervous. The caching of atl is definitly a benefit, but I think it makes just the initial start faster. What really counts is the incremental build speed and that should be as fast as possible.

Btw we could also introduce caching with webpack's cache-loader, especially when they provide a option for custom cache key generation.

I already tried the transpileOnly option of ts-loader with fork-ts-checker-webpack-plugin and the results are pretty promising. The only issue at the moment is that the type checking reporting isn't part of the webpack build workflow, so CRA's great error reporting in the terminal and browser will not work without any adaptations. However, this is solvable and we can workaround this.

@AJamesPhillips
Copy link
Contributor

AJamesPhillips commented Jun 15, 2017

@Havret @wmonk do you know if there are any benchmarks we can run before we merge this in? @plievone's and then @zinserjan's comments regarding slow watch speeds is getting me nervous... though as someone who has contributed nothing to this project yet I won't complain with whatever decision is collectively arrived at :) Just wanted to check all dissenting views had been engaged with as Plievone's seems to be have slipped through the net.

** edit ** Apologies, I also appreciate how annoying this might be after all the work you've generously put into this open source project.

@wmonk
Copy link
Owner

wmonk commented Jun 23, 2017

@zinserjan

I already tried the transpileOnly option of ts-loader with fork-ts-checker-webpack-plugin and the results are pretty promising.

I personally like this approach, as my editor provides the errors for me. Maybe we could make this a configurable option with an env var?

--

Can someone describe the non speed related benefits of using ATL? Is it just to allow babel plugins for language features (or addons, e.g. Relay) that TS doesn't support natively?

I'm aware this has been open for a long time now, sorry for not getting on it sooner! I want to make sure this is right decision before we move to it.

@zinserjan
Copy link
Contributor

@wmonk
The only issue with fork-ts-checker-webpack-plugin is that it does not play nicely with CRA's watch mode. fork-ts-checker-webpack-plugin works completly independent of webpacks compile lifecycle which produces the following issues:

  • type checking results will be logged to terminal and CRA clears it when webpack is slower than type checking, if webpack is faster than type checker everything is ok.
  • type checking error are not shown in the browser

In my personal opinion using fork-ts-checker-webpack-plugin with CRA is basically the same as disabling type checking completely, because you might never see the results.

But in general the performance gain of this plugin approach is pretty good. Just using the IDE (IntelliJ 2017) in my case isn't a option, cause type errors that affects multiple files are shown too late. I want the feedback immediately and fix those bugs when they appear and not when I'm already doing something else...

Another issue I noticed that it does not work with CSS-Modules (which is not part of CRA, but I configured it on my fork).

I tried to fork the plugin and fix my issues, but fixing was more complicated than I initially thought and it would result in a complete rewrite. That's why I've started to build my own plugin. The approach is similar but the implementation is different. My incremental compile times with this plugin are nearly identical as without the plugin. But I have just a small project to test with, it would be interesting how this scales on a large project. Anyway I'll try to finish it over the weekend.

@mattleff
Copy link
Contributor

@wmonk

Can someone describe the non speed related benefits of using ATL? Is it just to allow babel plugins for language features (or addons, e.g. Relay) that TS doesn't support natively?

Being able to use babel-plugin-relay is the only reason I'm using awesome-typescript-loader. I can't speak to the other benefits (or drawbacks) of ATL.

@DorianGrey
Copy link
Collaborator

The only issue with fork-ts-checker-webpack-plugin is that it does not play nicely with CRA's watch mode. fork-ts-checker-webpack-plugin works completly independent of webpacks compile lifecycle which produces the following issues:

  • type checking results will be logged to terminal and CRA clears it when webpack is slower than type checking, if webpack is faster than type checker everything is ok.
  • type checking error are not shown in the browser

The first issue seems to be caused by the change of version 0.2 which remove the option to forcefully block the emit. Maybe it's possible to convince the author(s) to reconsider this change, since it's not only a problem with CRA, but with webpack plugins like friendly-errors as well. Alternatively - that's my current workaround in a playground project - it's possible to use a custom logger which defers logging while webpack compilation is active. That wouldn't help with the second issue, however.

I have recently changed a production project (angular) with ~1000 typescript modules to use ts-loader in conjunction with fork-ts-checker-webpack-plugin instead of ATL, and the numbers got quite impressive:

  • Production build time (i.e. not affected by the first issue mentioned above): **~ -30%**
  • Dev build / rebuild time (theoretically suffers by the first issue, but practically never occurs due to project size): Differs, min. -30%, max. -75%

These numbers include the usage of TSLint as well - the plugin can handle it along with the type-checking, and share program instances, which especially helps with linter rules that also require type-checking.

Regarding the babel issue: I haven't recognized any problems or reasonable performance regressions in any project with using a proper chain of ts- and babel-loader instead of ATL, though I've only used this in projects with < 200 modules.

@KatSick
Copy link

KatSick commented Jul 31, 2017

One of the advantages of using at-loader is to be able to fight with microsoft/TypeScript#15088 those type of issues

@comerc
Copy link
Contributor

comerc commented Aug 20, 2017

FYI: Create-React-App + TypeScript + Ant-Design (without Eject) - how to change app from ts-loader to awesome-typescript-loader without eject.

Happy coding!

@wmonk
Copy link
Owner

wmonk commented Aug 22, 2017

Hi everyone - would like to get this merged or closed. What is the final consensus?

@corydeppen
Copy link

Assuming the two loaders are able to join forces (s-panferov/awesome-typescript-loader#266), is it worth moving to awesome-typescript-loader if the beneficial features are being ported to ts-loader and ts-loader will become the basis for the combined library?

@wmonk
Copy link
Owner

wmonk commented Aug 22, 2017

@corydeppen thanks for this, I hadn't seen it before. It doesn't look like there has been any movement there for a while though. Do you know of an update?

jthetzel added a commit to jthetzel/react-mapbox-gl that referenced this pull request Sep 28, 2017
Inclusion was causing TS6059 'rootDir' is expected
to contain all source files error. Suggestion to remove from:
wmonk/create-react-app-typescript#74
jthetzel added a commit to jthetzel/react-mapbox-gl that referenced this pull request Oct 3, 2017
Inclusion was causing TS6059 'rootDir' is expected
to contain all source files error. Suggestion to remove from:
wmonk/create-react-app-typescript#74
jthetzel added a commit to jthetzel/react-mapbox-gl that referenced this pull request Oct 4, 2017
Remove rootDir from example/tsconfig.json

Inclusion was causing TS6059 'rootDir' is expected
to contain all source files error. Suggestion to remove from:
wmonk/create-react-app-typescript#74

Change tsconfig.json compiler target to es7
With target set to es6, was receiving error:
ERROR in /home/jthetzel/src/react-mapbox-gl/src/layer.ts
(206,22): error TS2339: Property 'includes' does not exist on type 'number[]'.
Suggestion to try es7:
PatrickJS/PatrickJS-starter#931 (comment)

Removing carets from devDependencies
Presume this is preferred, as master branch does not
use carets.
jthetzel added a commit to jthetzel/react-mapbox-gl that referenced this pull request Oct 4, 2017
Remove rootDir from example/tsconfig.json

Inclusion was causing TS6059 'rootDir' is expected
to contain all source files error. Suggestion to remove from:
wmonk/create-react-app-typescript#74

Change tsconfig.json compiler target to es7
With target set to es6, was receiving error:
ERROR in /home/jthetzel/src/react-mapbox-gl/src/layer.ts
(206,22): error TS2339: Property 'includes' does not exist on type 'number[]'.
Suggestion to try es7:
PatrickJS/PatrickJS-starter#931 (comment)

Removing carets from devDependencies
Presume this is preferred, as master branch does not
use carets.
@bryanmig
Copy link

bryanmig commented Dec 5, 2017

@wmonk i suggest closing this

@Havret
Copy link
Author

Havret commented Dec 9, 2017

let it be

@Havret Havret closed this Dec 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.