Speed up TypeScript v2

[ianschmitz]

Let's try this again shall we? 😛

This is a continuation of the work done in #5903. Ideally this time around we can try to avoid any breaking changes on the react-dev-utils side of things if possible.

The initial commit here is simply reverting the revert. We can then work together to improve from here.

/cc @deftomat, @johnnyreilly, @Timer

[johnnyreilly]

Great! So what needs to happen to move things forward? From @ianschmitz comment:

I mistakenly published react-dev-utils as a patch level increase when it included the breaking change to https://github.com/facebook/create-react-app/pull/5903/files#diff-6736a5f1ee20761aaa6d0f4a09249e30, as well as a missing dependency of fork-ts-checker-webpack-plugin in the package.json of react-dev-utils. This caused issues for other packages that rely on the public API of react-dev-utils.

Is it just a matter of giving react-dev-utils a major version bump?

Or do we need to repeat the work of #6395 as well? (Which has also been reverted I think)

[Timer]

react-scripts patch with a react-dev-utils major should do the trick.

[johnnyreilly]

Great! Could you make that change at some point please @ianschmitz?

[ianschmitz]

Yep. A few thoughts:

• I think it'd be best if we convert the createCompiler() args to take a single args object as part of this breaking change. This should reduce the need for breaking changes in the future
• Are we okay with adding fork-ts-checker-webpack-plugin as a dependency of react-dev-utils?
• I think @Timer had some thoughts/concerns still about using async:true. Hopefully he can elaborate more

[ianschmitz]

Small detail @johnnyreilly, I noticed fork-ts-checker is still using babel-code-frame while we're using @babel/code-frame. Not sure if this could create formatting differences between us?

[johnnyreilly]

I don't think so as you're using your own formatter - not that one that's "in the box". (This is likely proved already by the fork-ts-checker-webpack-plugin-alt that create-react-app is presently using is likely using babel-code-frame too )

That said, we should probably update that dependency in fork-ts-checker-webpack-plugin at some point anyway.

[MattMorrisDev]

I noticed on react-scripts@2.1.4 that the start command didn't seem to do any type-checking unless I manually went into the webpack config and changed ForkTsCheckerWebpackPlugin's silent option to false. Is that going to be the case for 2.1.6 or whatever this gets released as?

[johnnyreilly]

I believe the errors are reported in browser in the overlay @MattMorrisDev. @deftomat can probably advise.

[deftomat]

@MattMorrisDev well, I’m not sure what was broken by this “failed” release but errors should be rendered as overlays.

[deftomat]

What’s the next step? Is there anything which I can do to finally bring this to TS users experiencing slow build times?

[ianschmitz]

I've made a few changes:

• createCompiler() now takes in an args object
• Moved fork-ts-checker-webpack-plugin to a dependency of react-dev-utils as well as created a re-export module the same as whate we've done with globby, chalk and others. This is now how it's consumed from our webpack config

Some outstanding questions:

• I'm thinking there must be a cleaner way we can provide a mechanism to write out warnings/errors to the dev server socket. See https://github.com/facebook/create-react-app/blob/007e6fd6190346bf1a74c23a57e2b5a172379f4d/packages/react-scripts/scripts/start.js#L99-L123 and notice that we use the closure when we create devSocket to reference the webpack dev server variable later created (devServer) that is created using the output from createCompiler(). This feels a little weird and is also only currently used for TypeScript builds, so not sure from a react-dev-utils public API perspective if this is what we want
• Do we still want async: true? I imagine we could avoid these changes with async: false, but not sure in real world projects what the speed difference would be. @Timer i know this was a concern of yours.

[deftomat]

👍 for the changes

• Yes, that closure is a little bit strange but it is probably the simplest way how to do it without changing a lot of things, especially the compilation hooks registration.

• About async:true: well, this was asked probably a million times, so I leave it to you 😉
  My only concern is that we should not assume that it is fast enough for everyone. This was the case with the first TypeScript PR and it was painfully slow for real world users.

[pelotom]

My only concern is that we should not assume that it is fast enough for everyone. This was the case with the first TypeScript PR and it was painfully slow for real word users.

Speaking as a real world user, async: true is definitely a must.

[johnnyreilly]

Speaking as a real world user, async: true is definitely a must.

I'd say async: false is only really desirable for production builds; where you want any errors to be registered with webpack so builds fail.

When you're in development / watch mode async: true is typically what you want for reasons of speed (essentially what this PR is all about 😊)

If there's a desire to use async: false in dev / watch mode it would be great to understand the rationale. (So we can be sure we make the choice for good reasons)

[ianschmitz]

Okay thanks for the feedback everyone. We will move forward with what we have now and release a breaking change to react-dev-utils.

Fingers crossed we don't need a v3 😉