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

update to typescript 4.5.5, and @typescript-eslint/parser 5.10.1 #4420

Merged
merged 3 commits into from
Feb 3, 2022

Conversation

warner
Copy link
Member

@warner warner commented Jan 30, 2022

Update to TS 4.5. Use a ~ instead of ^ since TS doesn't respect semver, and we want to avoid bumping "minor" versions inadvertently.

Also update @typescript-eslint/parser to 5.10.1, bump various eslint plugins versions, and groom yarn.lock to trim old eslint plugins
Force update eslint dependencies of react-scripts (through resolutions) to avoid upgrade to a breaking 5.0 version (unblocks #4430)
Drive-by remove of some egregious package duplicates

@warner warner requested a review from mhofman January 30, 2022 08:08
@warner warner force-pushed the warner-update-typescript branch from 7f12176 to bebfbfc Compare January 30, 2022 08:09
@warner
Copy link
Member Author

warner commented Jan 30, 2022

@mhofman any ideas how I should proceed?

@mhofman
Copy link
Member

mhofman commented Jan 31, 2022

Ok I had to update a few other eslint plugins and react-scripts to make it all work. Then I went chasing the TS issues that popped up and addressed them rather crudely (cast in most cases).

Looping in people to check the changes: @FUDCo @turadg @michaelfig @samsiegart

@warner
Copy link
Member Author

warner commented Jan 31, 2022

The swingset kernelKeeper changes look fine, they match things I had to do in a different branch (#4417). If the inline-ification swingset changes aren't necessary, I'd like to avoid them. The types picked for manager-helper.js are fine.

I think that covers the swingset parts of this PR.

Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

Left some suggestions. One non-blocking request: no explicit any and instead use ts-expect-error so that:

  • it's more conspicuous that there's an error to address
  • we don't erase type the info we do have
  • when the error is gone we are notified

packages/marshal/src/marshal-justin.js Outdated Show resolved Hide resolved
@mhofman
Copy link
Member

mhofman commented Jan 31, 2022

One non-blocking request: no explicit any and instead use ts-expect-error

In general I'd agree, but the rest of the file is untyped and cluttered with implicit any, so this is basically reverting behavior before TS decided to be smarter about inference.

@mhofman mhofman requested review from FUDCo and turadg January 31, 2022 23:33
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

LGTM all things considered. We can clean up the any in a separate effort.

Glad to see 4.5. I was just today running into the need for const in JSDoc https://devblogs.microsoft.com/typescript/announcing-typescript-4-5/#jsdoc-const-and-type-arg-defaults

@turadg
Copy link
Member

turadg commented Feb 1, 2022

Noticed this in the release notes: https://devblogs.microsoft.com/typescript/announcing-typescript-4-5/#module-es2022

Should we change the module format from esnext to es2020 to get a stable target? Not needed for this PR but a quick follow or perhaps a new ticket to capitalize on changes in 4.5?

@mhofman
Copy link
Member

mhofman commented Feb 2, 2022

I extracted the CI changes to #4433 as we realized the issue was independent, and rebased on top of that.

I also opened a PR on endo with the necessary patches to marshal and eventual send: endojs/endo#1043

Should we change the module format from esnext to es2020 to get a stable target?

I don't think so. AFAIK there has been no problem with tracking a moving target.

@samsiegart
Copy link
Contributor

Noting here for visibility, react-scripts 5.0 has some breaking changes that will affect on-chain wallet work:
When importing cosmjs:

Module not found: Error: Can't resolve 'path' in '/home/samsiegart/agoric-sdk/node_modules/libsodium/dist/modules'
BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.
https://github.com/facebook/create-react-app/issues/11756

Looks like they have a fix in progress facebook/create-react-app#11764, but for now we'll have to do a workaround instead of blocking this PR.

@mhofman
Copy link
Member

mhofman commented Feb 2, 2022

Looks like they have a fix in progress facebook/create-react-app#11764, but for now we'll have to do a workaround instead of blocking this PR.

let me see if I can undo CRA update

@mhofman mhofman force-pushed the warner-update-typescript branch from 61f0238 to a9f3176 Compare February 2, 2022 20:41
@mhofman
Copy link
Member

mhofman commented Feb 2, 2022

Reverted the update of react-scripts to 5.0 and instead forced eslint plugin resolutions to avoid conflicting versions. PTAL @samsiegart

@samsiegart
Copy link
Contributor

Reverted the update of react-scripts to 5.0 and instead forced eslint plugin resolutions to avoid conflicting versions. PTAL @samsiegart

Nice that worked: https://github.com/Agoric/agoric-sdk/pull/4438/commits
I got a weird error TypeError: [...ImportIssuer.jsx] helperSkipTransparentExpressionWrappers.skipTransparentExprWrapperNodes is not a function at first but nuking node_modules and rebuilding fixed it

@mhofman
Copy link
Member

mhofman commented Feb 3, 2022

@FUDCo rebased on your landed PR! Please check that the fixes to satisfy type checks are still acceptable.

Update @typescript-eslint/parser to 5.10.1
Bump various eslint plugins versions and groom yarn.lock to trim old eslint plugins
Force update eslint dependencies of react-scripts to avoid upgrade to breaking 5.0
Drive-by remove of some egregious package duplicates
Fix date format jest mock
No behavioral changes
@mhofman mhofman force-pushed the warner-update-typescript branch from 4449e2a to 9324cfe Compare February 3, 2022 18:33
Comment on lines +107 to +114
},
"resolutions": {
"**/react-scripts/@typescript-eslint/parser": "^5.10.2",
"**/react-scripts/@typescript-eslint/eslint-plugin": "^5.10.2",
"**/react-scripts/eslint-plugin-jest": "^26.0.0",
"**/react-scripts/eslint-plugin-testing-library": "^5.0.1",
"**/react-scripts/eslint-config-react-app": "^7.0.0",
"**/eslint-config-react-app/eslint-plugin-jest": "^26.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

@turadg @michaelfig any qualms about this approach to avoid eslint plugin duplication?

Copy link
Member

Choose a reason for hiding this comment

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

none that block it.

my thought though is that if we're going to control the versions at the root then to consider removing the dependencies from the package level. Some options:

  • Rely on the root package for these
  • Make a new package like common that has utilities used by many packages
  • Expand the eslint-config package to be responsible for eslint deps too

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately resolutions only work at the root with yarn. The problem is that we can't update react-scripts because of a breaking change, but it drags in eslint plugins as dependencies instead of devDependencies, and eslint plugins are notorious for causing issues when different versions lie around.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I misread this as overriding all eslint, not just the react-scripts ones. No concerns at all.

@mhofman mhofman added tooling repo-wide infrastructure automerge:no-update (expert!) Automatically merge without updates labels Feb 3, 2022
@mergify mergify bot merged commit 93dfb7c into master Feb 3, 2022
@mergify mergify bot deleted the warner-update-typescript branch February 3, 2022 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:no-update (expert!) Automatically merge without updates tooling repo-wide infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants