Skip to content

build(webpack): Use fork-ts webpack plugin #15587

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

Merged
merged 10 commits into from
Feb 5, 2020

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Nov 13, 2019

This should decrease build times since we are only running either
babel-loader or ts-loader and not both. We should skip typechecking for
local dev (and instead run type checking in a forked process) and always
run type checking in CI.

This will also skip typechecking when building prod assets (which should
be fine since we run them in CI).

👀👀

Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Neat. 👏 for faster local builds.

@billyvg billyvg added the WIP label Nov 14, 2019
@billyvg billyvg force-pushed the build/webpack/fork-typescript-type-check branch from 8d4d501 to 9d99e2f Compare December 11, 2019 01:00
@billyvg billyvg removed the WIP label Dec 11, 2019
@billyvg billyvg changed the title build(webpack): Use fork-ts webpack plugin [WIP] build(webpack): Use fork-ts webpack plugin Dec 11, 2019
@billyvg
Copy link
Member Author

billyvg commented Dec 11, 2019

OK tested this with wrong types and CI fails properly.

@billyvg billyvg requested a review from a team December 11, 2019 01:21
@billyvg billyvg force-pushed the build/webpack/fork-typescript-type-check branch from 34b5c86 to eb90818 Compare December 11, 2019 16:52
@BYK
Copy link
Member

BYK commented Dec 11, 2019

Do we have numbers for the speed up?

@billyvg
Copy link
Member Author

billyvg commented Dec 17, 2019

For TSX files (and incremental builds) I was seeing times from:

  • before: 4-10 seconds
  • after: 3-5 seconds for the initial build and an additional 3-5 seconds for type checking

@billyvg billyvg force-pushed the build/webpack/fork-typescript-type-check branch from eb90818 to 69e68b1 Compare December 17, 2019 19:12
@evanpurkhiser
Copy link
Member

What needs to be done to getsentry for this?

@billyvg
Copy link
Member Author

billyvg commented Jan 6, 2020

@evanpurkhiser Hmm what's the current state of ts in getsentry? We don't do type checking in there do we?

@evanpurkhiser
Copy link
Member

No we don't, but building sentry happens in getsentry, thus having typescript configs over there, so we may potentially need to mirror things over there.

@billyvg
Copy link
Member Author

billyvg commented Jan 6, 2020

We extend sentry config in getsentry though, so it should build sentry with sentry's webpack config, right?

@billyvg billyvg force-pushed the build/webpack/fork-typescript-type-check branch from 69e68b1 to 9f1b02f Compare January 7, 2020 00:16
@billyvg
Copy link
Member Author

billyvg commented Jan 8, 2020

I've tested this and it rebuilds in sentry (edit: getsentry) using the fork plugin fine

@billyvg billyvg force-pushed the build/webpack/fork-typescript-type-check branch 2 times, most recently from 3102919 to 06051a8 Compare January 13, 2020 19:44
@billyvg billyvg force-pushed the build/webpack/fork-typescript-type-check branch from 06051a8 to c8c3ead Compare January 27, 2020 19:13
@priscilawebdev
Copy link
Member

@billyvg please fix conflicts :)

transpileOnly: false,
},
},
...(!IS_CI
Copy link
Member

Choose a reason for hiding this comment

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

I kind of dislike this style prop spreading, I think it's a tad hard to read) but 🤷‍♂

I'd maybe prefer to have variables for like, tsLoaderConfig and bableLoaderConfig and then down here have

...(!IS_CI ? [bableLoaderConfig] : [tsLoaderConfig]),

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm the spread doesn't look like it's even necessary either

Copy link
Member

@evanpurkhiser evanpurkhiser left a comment

Choose a reason for hiding this comment

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

This looks good to me

@jeffkwoh
Copy link
Contributor

jeffkwoh commented Feb 4, 2020

Not sure if needed, but might want to check if similar needs to be done for getsentry.
Otherwise LGTM

@billyvg billyvg force-pushed the build/webpack/fork-typescript-type-check branch 2 times, most recently from b575a16 to e323b83 Compare February 5, 2020 00:42
@billyvg
Copy link
Member Author

billyvg commented Feb 5, 2020

Ugh Percy snapshots do not check out, holding off on mergin

@billyvg
Copy link
Member Author

billyvg commented Feb 5, 2020

This is the emotion problem we are facing: emotion-js/emotion#1748 - The last time I updated the PR was before the emotion 10 change, so we did not see these percy diffs.

Edit: looks like it's just an issue with a ts configuration (preserve JSX).

@billyvg billyvg force-pushed the build/webpack/fork-typescript-type-check branch from f365736 to 427474a Compare February 5, 2020 18:29
@billyvg billyvg force-pushed the build/webpack/fork-typescript-type-check branch from 427474a to 40a3dc8 Compare February 5, 2020 19:09
@billyvg billyvg merged commit 5ee4b3e into master Feb 5, 2020
@billyvg billyvg deleted the build/webpack/fork-typescript-type-check branch February 5, 2020 20:58
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants