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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up TypeScript v2 #6406

Merged
merged 4 commits into from Feb 19, 2019
Merged

Conversation

ianschmitz
Copy link
Contributor

@ianschmitz ianschmitz commented Feb 12, 2019

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

johnnyreilly commented Feb 12, 2019

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

Timer commented Feb 12, 2019

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

@johnnyreilly
Copy link
Contributor

johnnyreilly commented Feb 13, 2019

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

@ianschmitz
Copy link
Contributor Author

ianschmitz commented Feb 14, 2019

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

ianschmitz commented Feb 14, 2019

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

johnnyreilly commented Feb 14, 2019

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

MattMorrisDev commented Feb 15, 2019

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

johnnyreilly commented Feb 17, 2019

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

@deftomat
Copy link
Contributor

deftomat commented Feb 17, 2019

@MattMorrisDev well, I鈥檓 not sure what was broken by this 鈥渇ailed鈥 release but errors should be rendered as overlays.

@deftomat
Copy link
Contributor

deftomat commented Feb 17, 2019

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

@ianschmitz
Copy link
Contributor Author

ianschmitz commented Feb 17, 2019

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
    const devSocket = {
    warnings: warnings =>
    devServer.sockWrite(devServer.sockets, 'warnings', warnings),
    errors: errors =>
    devServer.sockWrite(devServer.sockets, 'errors', errors),
    };
    // Create a webpack compiler that is configured with custom messages.
    const compiler = createCompiler({
    appName,
    config,
    devSocket,
    urls,
    useYarn,
    useTypeScript,
    webpack,
    });
    // Load proxy config
    const proxySetting = require(paths.appPackageJson).proxy;
    const proxyConfig = prepareProxy(proxySetting, paths.appPublic);
    // Serve webpack assets generated by the compiler over a web server.
    const serverConfig = createDevServerConfig(
    proxyConfig,
    urls.lanUrlForConfig
    );
    const devServer = new WebpackDevServer(compiler, serverConfig);
    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
Copy link
Contributor

deftomat commented Feb 18, 2019

馃憤 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
Copy link
Contributor

pelotom commented Feb 18, 2019

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

johnnyreilly commented Feb 18, 2019

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 ianschmitz merged commit 6a5b3cd into facebook:master Feb 19, 2019
1 of 2 checks passed
@ianschmitz ianschmitz deleted the speedy-typescript branch Feb 19, 2019
@ianschmitz
Copy link
Contributor Author

ianschmitz commented Feb 19, 2019

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 馃槈

@lock lock bot locked and limited conversation to collaborators Feb 24, 2019
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.

None yet

8 participants