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

Take all updates that we can #1894

Merged
merged 1 commit into from
Nov 26, 2023
Merged

Take all updates that we can #1894

merged 1 commit into from
Nov 26, 2023

Conversation

ebroder
Copy link
Member

@ebroder ebroder commented Nov 23, 2023

The latest versions of mediasoup use the ??= operator, which isn't supported on Node 14, so we need to block dependabot updates. Additionally, our block on @types/node didn't allow us to take updates to the v14 types, so relax that slightly.

Additionally, drop p-throttle as a dependency, as we have not used it since c2d07b3.

Finally, some update-related housekeeping: add aria-labels to components in GuessQueuePage and Puzzle which have tooltips (which aren't typically used as labels by screenreaders). These aren't required with our current version of eslint-plugin-jsx-a11y, but it seems like they will likely be required in the future. (The latest verison does require them, although it also has a bug with table elements, so we are deferring that update for now.)

@ebroder ebroder requested a review from zarvox November 23, 2023 17:55
@ebroder ebroder enabled auto-merge November 23, 2023 17:55
Copy link
Contributor

@zarvox zarvox left a comment

Choose a reason for hiding this comment

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

Thanks for sorting all this out!

@zarvox
Copy link
Contributor

zarvox commented Nov 23, 2023

Looks like maybe the @typescript-eslint or typescript updates might not be happy?

#23 39.56 ../node_modules/@typescript-eslint/typescript-estree/dist/ts-estree/ts-nodes.d.ts(11,15): error TS2300: Duplicate identifier 'AssertClause'.
#23 39.56 ../node_modules/@typescript-eslint/typescript-estree/dist/ts-estree/ts-nodes.d.ts(13,15): error TS2300: Duplicate identifier 'AssertEntry'.
#23 39.57 ../node_modules/typescript/lib/typescript.d.ts(6021,10): error TS2300: Duplicate identifier 'AssertEntry'.
#23 39.57 ../node_modules/typescript/lib/typescript.d.ts(6023,10): error TS2300: Duplicate identifier 'AssertClause'.

@ebroder
Copy link
Member Author

ebroder commented Nov 23, 2023

Ugh yeah, I noticed this in #1876 and wasn't sure why I didn't notice it locally, but I think I didn't mash my way through enough of the install/lint/test commands because I'm seeing it now. I'll poke around a bit.

@ebroder
Copy link
Member Author

ebroder commented Nov 23, 2023

Looks like this is an incompatibility between TypeScript and typescript-eslint. (You can see a reference to it here: https://github.com/typescript-eslint/typescript-eslint/pull/7968/files#r1399942500). It'll probably get fixed in the latest release of typescript-eslint, but we can't pick that up due to Node v14 incompatibilities. So we can work around it by skipping validation of the type declarations used in the eslint plugin with the skipLibCheck option to tsconfig.

@zarvox
Copy link
Contributor

zarvox commented Nov 26, 2023

Looks like this is an incompatibility between TypeScript and typescript-eslint. (You can see a reference to it here: https://github.com/typescript-eslint/typescript-eslint/pull/7968/files#r1399942500). It'll probably get fixed in the latest release of typescript-eslint, but we can't pick that up due to Node v14 incompatibilities. So we can work around it by skipping validation of the type declarations used in the eslint plugin with the skipLibCheck option to tsconfig.

Was the force-push after this comment supposed to include the change you described (adding skipLibCheck to the tsconfig)? It looks like it was just a rebase

The latest versions of mediasoup use the `??=` operator, which isn't
supported on Node 14, so we need to block dependabot updates.
Additionally, our block on `@types/node` didn't allow us to take updates
to the v14 types, so relax that slightly.

Additionally, drop `p-throttle` as a dependency, as we have not used it
since c2d07b3.

Enable `skipLibCheck` on our eslint plugin - this causes tsc to
typecheck our plugin code but take library types as accurate. This is
necessary because our version of typescript-eslint has some
incompatibilities with the latest version of TypeScript, and we can't
currently update typescript-eslint due to Node v14 incompatibilities.

Finally, some update-related housekeeping: add aria-labels to components
in `GuessQueuePage` and `Puzzle` which have tooltips (which aren't
typically used as labels by screenreaders). These aren't required with
our current version of `eslint-plugin-jsx-a11y`, but it seems like they
will likely be required in the future. (The latest verison does require
them, although it also has a bug with table elements, so we are
deferring that update for now.)
@ebroder
Copy link
Member Author

ebroder commented Nov 26, 2023

Was the force-push after this comment supposed to include the change you described (adding skipLibCheck to the tsconfig)? It looks like it was just a rebase

Ah yes it was, whoops. Should be there now.

@ebroder ebroder merged commit effe830 into main Nov 26, 2023
1 check passed
@ebroder ebroder deleted the evan/updates branch November 26, 2023 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants