Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
This repository was archived by the owner on Jan 26, 2019. It is now read-only.

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

Closed
Havret wants to merge15 commits intowmonk:masterfromHavret:master
Closed

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

Havret wants to merge15 commits intowmonk:masterfromHavret:master

Conversation

@Havret
Copy link

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

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

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

@bryanmigbryanmig 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 commentedMay 25, 2017
edited
Loading

[at-loader] Checking finished with 1116 errorsError 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

@KatSickKatSick 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 commentedMay 27, 2017
edited
Loading

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

@Havret
Copy link
Author

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 and jthetzel reacted with hooray emoji

@Havret
Copy link
Author

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

D1no reacted with thumbs up emoji

@wmonk
Copy link
Owner

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

@plievone
Copy link

Hi, if you proceed with this, please measure some numbers (also withwebpack --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 withtranspileOnly: 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.

Guria, AJamesPhillips, and nizsheanez reacted with thumbs up emoji

@mattleff
Copy link
Contributor

@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 addingconfig to theexclude key (seehere) in yourtsconfig.json. That will work with therootDir present.rootDir is necessary for cases where there are multiple Typescript projects in the same tree (such as a submodule).

@mattleff
Copy link
Contributor

@wmonk@Havret I can confirm that addingconfig works fine. You can check out my diffhere. With this change I am able to usebabel-plugin-relay. 🎉

@Havret
Copy link
Author

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

@zinserjan
Copy link
Contributor

Isawesome-typescript-loader really the way to go?

I just read the notes about the poor performance for incremental builds with atl atfork-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'scache-loader, especially when they provide a option for custom cache key generation.

I already tried thetranspileOnly option of ts-loader withfork-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 and mkozhukharenko reacted with thumbs up emoji

@AJamesPhillips
Copy link
Contributor

AJamesPhillips commentedJun 15, 2017
edited
Loading

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

@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 myown 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 usebabel-plugin-relay is the only reason I'm usingawesome-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 version0.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 likefriendly-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 usets-loader in conjunction withfork-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 thebabel 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

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

@comerc
Copy link
Contributor

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

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 andts-loader will become the basis for the combined library?

@wmonk
Copy link
Owner

@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 requestSep 28, 2017
Inclusion was causing TS6059 'rootDir' is expectedto 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 requestOct 3, 2017
Inclusion was causing TS6059 'rootDir' is expectedto 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 requestOct 4, 2017
Remove rootDir from example/tsconfig.jsonInclusion was causing TS6059 'rootDir' is expectedto contain all source files error. Suggestion to remove from:wmonk/create-react-app-typescript#74Change tsconfig.json compiler target to es7With 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 devDependenciesPresume this is preferred, as master branch does notuse carets.
jthetzel added a commit to jthetzel/react-mapbox-gl that referenced this pull requestOct 4, 2017
Remove rootDir from example/tsconfig.jsonInclusion was causing TS6059 'rootDir' is expectedto contain all source files error. Suggestion to remove from:wmonk/create-react-app-typescript#74Change tsconfig.json compiler target to es7With 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 devDependenciesPresume this is preferred, as master branch does notuse carets.
@bryanmig
Copy link

@wmonk i suggest closing this

@Havret
Copy link
Author

let it be

@HavretHavret closed thisDec 9, 2017
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

2 more reviewers

@KatSickKatSickKatSick left review comments

@bryanmigbryanmigbryanmig approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

11 participants

@Havret@bryanmig@wmonk@plievone@mattleff@zinserjan@AJamesPhillips@DorianGrey@KatSick@comerc@corydeppen

[8]ページ先頭

©2009-2025 Movatter.jp