Add TypeScript to the build system #13489
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
mcmire
force-pushed
the
add-typescript-to-build-system
branch
from
a1c8ea1 to
5bf6bee
Compare
mcmire
changed the base branch from
develop
to
add-typescript-to-eslint-config
mcmire
commented
Feb 15, 2022
| "@babel/plugin-transform-runtime": true, | ||
| "@babel/preset-env": true, | ||
| "@babel/preset-react": true, | ||
| "@babel/preset-typescript": true |
There was a problem hiding this comment.
I re-alphabetized this list, but @babel/preset-typescript is the new entry here.
mcmire
changed the title
mcmire
force-pushed
the
add-typescript-to-eslint-config
branch
from
8842d00 to
c0852fe
Compare
mcmire
force-pushed
the
add-typescript-to-build-system
branch
from
5bf6bee to
58e8669
Compare
mcmire
changed the title
mcmire
force-pushed
the
add-typescript-to-eslint-config
branch
from
c0852fe to
8d473ee
Compare
mcmire
force-pushed
the
add-typescript-to-build-system
branch
4 times, most recently
from
4f884f6 to
b2d7275
Compare
mcmire
commented
Feb 16, 2022
| @@ -1,7 +1,11 @@ | |||
| module.exports = function (api) { | |||
| api.cache(false); | |||
| return { | |||
| parserOpts: { | |||
| strictMode: true, | |||
There was a problem hiding this comment.
Make sure TypeScript files are parsed using strict mode. See https://babeljs.io/docs/en/babel-plugin-transform-typescript#typescript-compiler-options.
Builds ready [b2d7275]Page Load Metrics (1182 ± 32 ms)
|
Builds ready [4fe05c8]Page Load Metrics (1113 ± 25 ms)
|
mcmire
force-pushed
the
add-typescript-to-eslint-config
branch
3 times, most recently
from
e91257c to
93d8f12
Compare
mcmire
force-pushed
the
add-typescript-to-build-system
branch
from
4fe05c8 to
9b822d3
Compare
mcmire
force-pushed
the
add-typescript-to-eslint-config
branch
from
93d8f12 to
0d855e4
Compare
mcmire
force-pushed
the
add-typescript-to-build-system
branch
from
9b822d3 to
3bfd3a9
Compare
Builds ready [3bfd3a9]Page Load Metrics (1236 ± 44 ms)
|
mcmire
force-pushed
the
add-typescript-to-eslint-config
branch
from
0d855e4 to
de92b97
Compare
mcmire
force-pushed
the
add-typescript-to-build-system
branch
2 times, most recently
from
ade7789 to
3266b8a
Compare
Builds ready [3266b8a]Page Load Metrics (1467 ± 62 ms)
|
This commit modifies the build system so that TypeScript files can be transpiled into ES5 just like JavaScript files. Note that this commit does NOT change the build system to run TypeScript files through the TypeScript compiler. In other words, no files will be type-checked at the build stage, as we expect type-checking to be handled elsewhere (live, via your editor integration with `tsserver`, and before a PR is merged, via `yarn lint`). Rather, we merely instruct Babel to strip TypeScript-specific syntax from any files that have it, as if those files had been written using JavaScript syntax alone. Why take this approach? Because it prevents the build process from being negatively impacted with respect to performance (as TypeScript takes a significant amount of time to run). It's worth noting the downside of this approach: because we aren't running files through TypeScript, but relying on Babel's [TypeScript transform][1] to identify TypeScript syntax, this transform has to keep up with any syntax changes that TypeScript adds in the future. In fact there are a few syntactical forms that Babel already does not recognize. These forms are rare or are deprecated by TypeScript, so I don't consider them to be a blocker, but it's worth noting just in case it comes up later. Also, any settings we place in `tsconfig.json` will be completely ignored by Babel. Again, this isn't a blocker because there are some analogs for the most important settings reflected in the options we can pass to the transform. These and other caveats are detailed in the [documentation for the transform][2]. [1]: https://babeljs.io/docs/en/babel-plugin-transform-typescript [2]: https://babeljs.io/docs/en/babel-plugin-transform-typescript#caveats
This reverts commit 173af2a.
mcmire
force-pushed
the
add-typescript-to-build-system
branch
from
471fa31 to
aa802f5
Compare
Builds ready [aa802f5]Page Load Metrics (1406 ± 36 ms)
|
Builds ready [79740fd]Page Load Metrics (1264 ± 19 ms)
|
Builds ready [628d4e9]Page Load Metrics (1455 ± 96 ms)
|
digiwand
approved these changes
Mar 28, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit modifies the build system so that TypeScript files can be
transpiled into ES5 just like JavaScript files.
Note that this commit does NOT change the build system to run TypeScript
files through the TypeScript compiler. In other words, no files will be
type-checked at the build stage, as we expect type-checking to be
handled elsewhere (live, via your editor integration with
tsserver,and before a PR is merged, via
yarn lint). Rather, we merely instructBabel to strip TypeScript-specific syntax from any files that have it,
as if those files had been written using JavaScript syntax alone.
Why take this approach? Because it prevents the build process from being
negatively impacted with respect to performance (as TypeScript takes a
significant amount of time to run).
It's worth noting the downside of this approach: because we aren't
running files through TypeScript, but relying on Babel's TypeScript
transform to identify TypeScript syntax, this transform has to keep
up with any syntax changes that TypeScript adds in the future. In fact
there are a few syntactical forms that Babel already does not recognize.
These forms are rare or are deprecated by TypeScript, so I don't
consider them to be a blocker, but it's worth noting just in case it
comes up later. Also, any settings we place in
tsconfig.jsonwill becompletely ignored by Babel. Again, this isn't a blocker because there
are some analogs for the most important settings reflected in the
options we can pass to the transform. These and other caveats are
detailed in the documentation for the transform.
Manual testing steps:
git cherry-pick 9af9b6bbf1b26df408a40101e7eb8e727d334e05).ui/helpers/utils/tx-helper.jsandui/helpers/utils/storage-helpers.ts. Both of these files have dependents which are JavaScript files, andui/helpers/utils/tx-helper.jshas dependencies which are JavaScript files.yarn setup.ui/helpers/utils/storage-helpers.tsin your editor. Remove the semicolon from the first line; your editor should tell you (via ESLint) that this is invalid.ui/index.js. This file importstx-helper.ts. Mouse overtxHelperin theimport txHelperline; you should see proper types for this function which come from the TypeScript file.yarn lint. All files should pass.yarn start. You shouldn't see any errors.yarn dist. You shouldn't see any errors.dist/chrome/common-2.jsand search for "getStorageItem" (one of the functions inui/helpers/utils/storage-helpers.ts). Warning: this is a big file. You should find a reference to the function here.