-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Add TypeScript to the linting process #13495
Conversation
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. |
61890a6
to
5fbe15c
Compare
6b9fe58
to
1658fe3
Compare
Builds ready [1658fe3]Page Load Metrics (1267 ± 29 ms)
|
// <https://github.com/alexgorbatchev/eslint-import-resolver-typescript> | ||
'import/no-unresolved': 'error', | ||
|
||
// Disabled due to incompatibility with Record<string, unknown>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These settings are copied from the ESLint config in controllers
: https://github.com/MetaMask/controllers/blob/81bcbae1c4abe71a9b07819606e155a33ec4a6b4/.eslintrc.js#L29-L43. We should probably integrate them into our shared config.
@@ -46,3 +46,6 @@ notes.txt | |||
.nyc_output | |||
|
|||
.metamaskrc | |||
|
|||
# TypeScript | |||
tsout/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See note about tsout
in tsconfig.json
.
return to.concat(ar || Array.prototype.slice.call(from)); | ||
}; | ||
var __assign = (this && this.__assign) || function () { | ||
- __assign = Object.assign || function(t) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes to the typescript
package were motivated by failures I was getting from LavaMoat when attempting to generate LavaMoat policy files for the build system (via yarn lavamoat:auto
. That said, this commit is concerned with adding TypeScript to ESLint, not the build system, so why are these changes necessary?
Because, as I learned, the build system actually runs ESLint. As I've been going through the codebase, I've occasionally come across sections of code that are surrounded with ///: BEGIN:ONLY_INCLUDE_IN
and ///: END:ONLY_INCLUDE_IN
(for example, account-menu.component.js
. I've always wondered about them, and now I know that they get removed during a step in the build process. After this transform is applied to a file, that file is linted.
So, when running yarn lavamoat:auto
, LavaMoat sees these files and follows the path to eslint.config.js
, then follows the path to the typescript
package. Why does it trip up on this file in particular?
One of the things that LavaMoat, or more accurately the lockdown
function in SES, does is to protect against unwanted and unexpected modifications of global objects, such as Object.prototype
. Admittedly I am unable to explain much beyond this, but you can learn more about this function here. The lockdown
function, as it uses Object.freeze
to lock down objects, suffers from a "mistake" in the ECMAScript spec where a property inherited from a frozen object is itself frozen. Plain objects inherit constructor
from Object.prototype
, so if Object.prototype
is frozen, this code will not work:
const foo = {};
foo["constructor"] = "bar";
But that is what is happening in this file. The ts.textToKeywordObj
object maps names of keywords in TypeScript to token ids that represent those keywords (defined elsewhere), and one of the keywords in TypeScript is constructor
. For some reason, whatever ended up producing this file (probably TypeScript itself) defined this object essentially like this:
ts.textToKeywordObj = (
_a = { /* ... */ },
_a["constructor"] = 133,
// ...
_a
);
That doesn't work, so one of the changes made here is to clean up the definition of this object.
The other change is to the __assign
function, which you can see here. One of the ways that __assign
is used is to make a textToToken
variable on line 9904. This looks something like:
var textToToken = __assign(
{},
ts.textToKeywordObj,
{
"{": 18 /* OpenBraceToken */,
"}": 19 /* CloseBraceToken */,
// ...
}
)
As you can see here, __assign
defaults to Object.assign
here if it exists, or otherwise falls back to a polyfill of Object.assign
. Whether or not the polyfill is used, __assign
creates a new object by iterating over the given objects and manually copying their keys and values to the new object by setting them directly on the new object. This will attempt to set constructor
on the new object again, which as we've seen, is not allowed. So this change fixes __assign
by using Object.defineProperty
to set properties instead of direct assignment.
"inlineSources": true, | ||
"isolatedModules": true, | ||
"jsx": "react", | ||
"lib": ["dom", "es2020"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These settings were copied from the tsconfig.json
in controllers
but then adjusted to account for JSX as well as browser globals. ES2020 was chosen because Node v14 supports it, so that should make it less surprising for developers who wish to be able to write frontend code in a similar fashion as Node code. (The JavaScript that TypeScript generates will end up getting processed by Babel in the build system so we should be safe here.)
"moduleResolution": "node", | ||
"noUnusedLocals": true, | ||
"noUnusedParameters": true, | ||
"outDir": "tsout", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setting will not end up getting used by the build system. This is only here so that if you want to debug an issue you're seeing with TypeScript, you can say yarn tsc --project .
like this and inspect the generated JavaScript output. tsout
will be gitignored.
|
8a2321b
to
4b74949
Compare
Builds ready [5ffcffe]Page Load Metrics (1162 ± 63 ms)
|
5ffcffe
to
c4ca8e1
Compare
4b74949
to
c983c6c
Compare
c4ca8e1
to
8842d00
Compare
'.ts', | ||
'.tsx', | ||
- '.d.ts' | ||
-], Object.keys(require.extensions)), [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I opened this file, I got an ESLint warning that require.extensions
was deprecated. When I ran yarn lavamoat:auto
, then require.extensions
was undefined. I am not sure what exactly the correct fix here is, but I don't think we need to make this complicated — the list of extensions we want to support are JavaScript and TypeScript, so we don't need to use require.extensions
.
8842d00
to
c0852fe
Compare
c983c6c
to
fd16fdf
Compare
c0852fe
to
8d473ee
Compare
@@ -0,0 +1,1830 @@ | |||
diff --git a/node_modules/@keystonehq/bc-ur-registry/src/Bytes.ts b/node_modules/@keystonehq/bc-ur-registry/src/Bytes.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @keystonehq/bc-ur-registry
package contains TypeScript files in its distributed form. This is not necessary or recommended for TypeScript-compatible libraries (rather, it is enough to publish type definition files). The issue for this is logged here: KeystoneHQ/ur-registry#13. In the meantime, as our code works fine presently without any TypeScript files, we do not need these in order to use this library. This makes the new tsc
step in yarn lint
pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, to make sure I understand correctly, this code is not used by the extension, but we need this patch to get lint passing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry maybe my explanation was confusing. We do use this package (it looks like a component that is involved with the QR code stuff imports @keystonehq/bc-ur-registry-eth
, which imports @keystonehq/bc-ur-registry
). However, the way this package has been bundled and published to NPM is incorrect as it includes TypeScript files.
@@ -0,0 +1,68 @@ | |||
diff --git a/node_modules/await-semaphore/index.ts b/node_modules/await-semaphore/index.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The await-semaphore
package contains TypeScript files in its distributed form. This is not necessary or recommended for TypeScript-compatible libraries (rather, it is enough to publish type definition files). The issue for this is logged here: notenoughneon/await-semaphore#6. In the meantime, as our code works fine presently without any TypeScript files, we do not need these in order to use this library. This makes the new tsc
step in yarn lint
pass.
(I notice there is a thumbs-up on this issue from Mark so I'm curious if we ran into this issue in controllers
or mobile?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, to make sure I understand correctly, this code is not used by the extension, but we need this patch to get lint passing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do use this package (it looks like at the top of MetamaskController). However, the way this package has been bundled and published to NPM is incorrect as it includes TypeScript files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we're removing the Semaphore class that Mutex extends in this patch. Does this not break when this.createVaultMutex.acquire();
runs in the MetamaskController?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we aren't removing all the code in await-semaphore
. You can see here that the version of this package published to NPM includes both JavaScript and TypeScript files. We are just removing the TypeScript files, but we are still using the JavaScript files. So this.createVaultMutex.acquire();
should still work as before.
Builds ready [f6008cb]Page Load Metrics (1084 ± 65 ms)
|
Builds ready [93d8f12]Page Load Metrics (1117 ± 29 ms)
|
93d8f12
to
0d855e4
Compare
Builds ready [0d855e4]Page Load Metrics (1211 ± 36 ms)
|
This commit allows developers to write TypeScript files and lint them (either via a language server in their editor of choice or through the `yarn lint` command). The new TypeScript configuration as well as the updated ESLint configuration not only includes support for parsing TypeScript files, but also provides some compatibility between JavaScript and TypeScript. That is, it makes it possible for a TypeScript file that imports a JavaScript file or a JavaScript file that imports a TypeScript file to be linted. Note that this commit does not integrate TypeScript into the build system yet, so we cannot start converting files to TypeScript and pushing them to the repo until that final step is complete.
0d855e4
to
de92b97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look great to me, and thank you so much for all the code commenting! Hugely useful!
Builds ready [de92b97]Page Load Metrics (1438 ± 50 ms)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super! 🙌 I had one question about one of the patches and some other optional comments.
Thanks for walking through this PR with me last Friday and enlightening me on TypeScript + patching files in relation to our codebase.
Looking forward to removing the following paths from the excluded list someday too:
"**/*.test.js",
"**/*.test.ts",
"**/*.test.tsx",
".storybook/**/*",
'import/named': 'off', | ||
'import/namespace': 'off', | ||
'import/default': 'off', | ||
'import/no-named-as-default-member': 'off', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this since the rule lives in typescript-compat.js
. Is there a reason we are keeping this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, although arguably it shouldn't be in typescript-compat.js
as it shouldn't apply to every kind of file, just TypeScript files. Removed in cec7044.
"outDir": "tsout", | ||
"rootDir": ".", | ||
"sourceMap": true, | ||
"strict": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking a quick peek of some other tsconfig.json file configs for chrome-extensions:
- https://github.com/chibat/chrome-extension-typescript-starter/blob/master/tsconfig.json
- https://betterprogramming.pub/creating-chrome-extensions-with-typescript-914873467b65
I think you might have explained this last Friday, but I forgot. Sorry if this was the case. Why are we not including
"module": "commonjs",
"target": "es6",
again?
"noEmitOnError" - might be something to consider later. This holds back from generating js files to outDir
if there are ts errors found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might have explained this last Friday, but I forgot. Sorry if this was the case. Why are we not including
"module": "commonjs", "target": "es6",
We are, but I admit it's a bit hidden: that is being done by the "extends": "@tsconfig/node14/tsconfig.json"
line. You can see the source for that here.
"noEmitOnError" - might be something to consider later. This holds back from generating js files to outDir if there are ts errors found
I don't mind adding it now! Added in 0ccb400
@@ -0,0 +1,68 @@ | |||
diff --git a/node_modules/await-semaphore/index.ts b/node_modules/await-semaphore/index.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we're removing the Semaphore class that Mutex extends in this patch. Does this not break when this.createVaultMutex.acquire();
runs in the MetamaskController?
@digiwand Hopefully I've addressed all your concerns! Would you mind taking another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good! Thank you.
Last question I have: Is it possible to add comments to patch files? I think it would be helpful to provide context for each patch.
We can't add comments to file that we delete, but we can add them to those that we change (a patch file changes the files in question, so the comments get added to those files). I've done this in 8908c09. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thank you for this! 🎉
@darkwing May I get one more approval? |
Builds ready [f55c164]Page Load Metrics (1319 ± 57 ms)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit allows developers to write TypeScript files and lint them
(either via a language server in their editor of choice or through the
yarn lint
command).The new TypeScript configuration as well as the updated ESLint
configuration not only includes support for parsing TypeScript files,
but also provides some compatibility between JavaScript and TypeScript.
That is, it makes it possible for a TypeScript file that imports a
JavaScript file or a JavaScript file that imports a TypeScript file to
be linted.
Note that this commit does not integrate TypeScript into the build
system yet, so we cannot start converting files to TypeScript and
pushing them to the repo until that final step is complete.
Manual testing steps:
git cherry-pick 4bde1eb1d0bf0e84d76f83bb0bb065cfc1f45e58
.ui/helpers/utils/tx-helper.ts
andui/helpers/utils/storage-helpers.ts
. Both of these files have dependents which are JavaScript files, andui/helpers/utils/tx-helper.ts
has dependencies which are JavaScript files.yarn setup
.ui/helpers/utils/storage-helpers.ts
in 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 overtxHelper
in theimport txHelper
line; you should see proper types for this function which come from the TypeScript file.yarn lint
. All files should pass.