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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade Typescript to v5 #467

Merged
merged 14 commits into from
Sep 9, 2024
Merged

Upgrade Typescript to v5 #467

merged 14 commits into from
Sep 9, 2024

Conversation

sisou
Copy link
Member

@sisou sisou commented Mar 19, 2023

v4 -> v5 was no changes actually, see the commit history.

This is based on the usdc branch, so can only be merged after #465 is merged.

@sisou sisou requested review from danimoh and mraveux March 19, 2023 19:35
@sisou sisou self-assigned this Mar 19, 2023
Base automatically changed from usdc to master October 22, 2023 14:09
@sisou sisou mentioned this pull request Aug 19, 2024
@sisou sisou force-pushed the soeren/ts-update branch 2 times, most recently from f275cab to d41923b Compare August 24, 2024 08:54
Copy link
Member

@onmax onmax left a comment

Choose a reason for hiding this comment

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

LGTM

this.$fields[index].fillValueFrom(paste);
const field = this.$fields[index];
field.focus();
if (paste && 'fillValueFrom' in field) {
Copy link
Member

@onmax onmax Aug 29, 2024

Choose a reason for hiding this comment

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

Suggested change
if (paste && 'fillValueFrom' in field) {
if (paste && 'fillValueFrom' in field && typeof field.fillValueFrom === 'function') {

not sure if this is relevant

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is necessary.

src/components/RecoveryWords.js Outdated Show resolved Hide resolved
tsconfig.json Outdated
@@ -1,8 +1,8 @@
{
"compilerOptions": {
/* Basic Options */
"target": "ES2015", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017','ES2018' or 'ESNEXT'. */
"module": "commonjs", /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */
"target": "ES2020", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017','ES2018' or 'ESNEXT'. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"target": "ES2020", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017','ES2018' or 'ESNEXT'. */
"target": "ES2020", /* Specify ECMAScript target version. */

Copy link
Member

@onmax onmax Aug 29, 2024

Choose a reason for hiding this comment

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

People can learn what values are valid from the docs so we don't have these comments out of sync ?

"target": "ES2015", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017','ES2018' or 'ESNEXT'. */
"module": "commonjs", /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */
"target": "ES2020", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017','ES2018' or 'ESNEXT'. */
"module": "CommonJS", /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"module": "CommonJS", /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */
"module": "CommonJS", /* Specify module code generation. */

Copy link
Member

@danimoh danimoh left a comment

Choose a reason for hiding this comment

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

Cool, thanks for this.
I added my improvements, feel free to have a look.
The change of static properties of classes to actual js statics requires an eslint update, so this will be in a separate PR.

@onmax please also review my changes, if you want.

@danimoh
Copy link
Member

danimoh commented Sep 6, 2024

The force-push is for rebase onto master, with resolved conflicts in the FiatApi.
However, some additional type errors seem to have surfaced on the latest changes from master.
Taking a look now.

src/lib/LoginFile.js Outdated Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
src/common.js Outdated Show resolved Hide resolved
src/components/PasswordBox.js Show resolved Hide resolved
src/components/PasswordSetterBox.js Show resolved Hide resolved
src/components/PasswordSetterBox.js Show resolved Hide resolved
sisou and others added 12 commits September 6, 2024 13:11
…pe clashs

Typecheck service worker separately with ts WebWorker lib, and the rest of the Keyguard
with ts DOM lib. Previously, both libs were active in all files (even though webworker
was referenced only in the ServiceWorker via a triple slash directive, this enables it
globally), which by itself is inappropriate, but also causes type clashs between the
DOM and the WebWorker types, which previously were silenced by skipping lib checks.

Now, with this change, and typescript having been updated to a recent version, checking
libs has been enabled again.
Browser support for padEnd is now sufficiently good.
Allow them to be passed as null or undefined everywhere.
danimoh and others added 2 commits September 6, 2024 13:20
This was already the case before updating typescript to the latest v3 version in 35859ed,
which temporarily removed that handling as is was not compatible with newer typescript
versions. This commit reimplements this behavior in a different fashion.
For readability and easier understanding.
@sisou sisou merged commit 4e68e59 into master Sep 9, 2024
2 checks passed
@sisou sisou deleted the soeren/ts-update branch September 9, 2024 13:35
sisou added a commit that referenced this pull request Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants