microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.15k stars 12.51k forks source link

Typescript 5.5 does not infer event handler types in MUI #59474

Open marc-at-brightnight opened 3 months ago

marc-at-brightnight commented 3 months ago

🔎 Search Terms

"inferred types event handler"

🕗 Version & Regression Information

âŊ Playground Link

Code Sandbox

ðŸ’ŧ Code

// Your code here

import "./styles.css";
import { IconButton } from '@mui/material'
import ContentCopyIcon from '@mui/icons-material/ContentCopy';

export default function App() {

  const handleCopyClick = (event: React.MouseEvent<HTMLElement>, id: string | null) => {
    event.stopPropagation();
    console.log('hello', id)
  }

  return (
    <div className="App">
      <IconButton onClick={(event) => handleCopyClick(event, 'id')}>
        <ContentCopyIcon />
      </IconButton>
    </div>
  );
}

🙁 Actual behavior

in onClick={(event) => ..., event is underlined with TS error 'Parameter 'event' implicitly has an 'any' type.ts(7006)'

Typing the event handler with onClick={(event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => ... makes this error go away

🙂 Expected behavior

I would expect the type of event to be inferred from the underlying onClick event handler. This was the previous behavior of Typescript.

Short of a fix to this, I would like to at least understand why this changed and why explicitly typing event improves type safety/robustness.

Additional information about the issue

image

RyanCavanaugh commented 3 months ago

The StackBlitz doesn't have the reproing code in it, and I think the code you have in the issue isn't quite right? You have

import IconButton from '@mui/material'

but it should be

import { IconButton } from '@mui/material'

I'm not seeing any error once I correct that problem

jakebailey commented 3 months ago

Yes, I just tested this too; note that the screenshot says "3 problems":

src/index.tsx:16:8 - error TS2604: JSX element type 'IconButton' does not have any construct or call signatures.

16       <IconButton onClick={(event) => handleCopyClick(event, 'id')}>
          ~~~~~~~~~~

src/index.tsx:16:8 - error TS2786: 'IconButton' cannot be used as a JSX component.
  Its type 'typeof import("/home/jabaile/work/repros/issue59474/node_modules/@mui/material/index")' is not a valid JSX element type.

16       <IconButton onClick={(event) => handleCopyClick(event, 'id')}>
          ~~~~~~~~~~

src/index.tsx:16:29 - error TS7006: Parameter 'event' implicitly has an 'any' type.

16       <IconButton onClick={(event) => handleCopyClick(event, 'id')}>
                               ~~~~~

The implict any is the least of the problems

marc-at-brightnight commented 3 months ago

Whoops, seems I was a bit hasty when I put together the example ðŸĪŠ

Corrected example here

@RyanCavanaugh you are correct, there is no more issue and the types are inferred correctly. Now I'm completely at a loss, as I have essentially the same code with the same package versions in my production code (screenshot below) but staring at that error as clear as day... I will have to investigate more to see if I can reproduce the issue but definitely seems like an issue with my app and not Typescript (thank god!). (and yes, my imports are correct 😅)

image

jakebailey commented 3 months ago

If you're certain this is a regression in 5.5, consider using https://www.npmjs.com/package/every-ts to bisect tsc to figure out where it changed.

marc-at-brightnight commented 3 months ago

@jakebailey thanks, great idea! Definitely seems like 5.5 is the issue, seems to work on 5.4... image

jakebailey commented 3 months ago

To be clear, I mean using every-ts bisect: https://www.npmjs.com/package/every-ts#Bisecting

marc-at-brightnight commented 3 months ago

To be clear, I mean using every-ts bisect: https://www.npmjs.com/package/every-ts#Bisecting

I gotcha. Is this what you meant?

poweralpha-app/packages/ui on  upgrade-typescript-to-5.5 [$] via  v20.12.0 on ☁ïļ  default
âŊ every-ts bisect start
status: waiting for both good and bad commits

poweralpha-app/packages/ui on  upgrade-typescript-to-5.5 [$] via  v20.12.0 on ☁ïļ  default
âŊ every-ts bisect good 5.4
Resolved 5.4 to origin/release-5.4
status: waiting for bad commit, 1 good commit known

poweralpha-app/packages/ui on  upgrade-typescript-to-5.5 [$] via  v20.12.0 on ☁ïļ  default
âŊ every-ts bisect bad 5.5
Resolved 5.5 to origin/release-5.5
Bisecting: a merge base must be tested
remote: Enumerating objects: 468, done.
remote: Counting objects: 100% (445/445), done.
remote: Compressing objects: 100% (213/213), done.
remote: Total 468 (delta 283), reused 232 (delta 232), pack-reused 23
Receiving objects: 100% (468/468), 1.33 MiB | 6.38 MiB/s, done.
Resolving deltas: 100% (285/285), done.
[d04e3489b0d8e6bc9a8a9396a633632a5a467328] Improve apparent type of mapped types (#57122)
Building TypeScript...
TypeScript built successfully!

poweralpha-app/packages/ui on  upgrade-typescript-to-5.5 [$] via  v20.12.0 on ☁ïļ  default took 23s
âŊ every-ts tsc --version
Version 5.4.0-dev

poweralpha-app/packages/ui on  upgrade-typescript-to-5.5 [$] via  v20.12.0 on ☁ïļ  default
âŊ every-ts tsc --noEmit -p .

poweralpha-app/packages/ui on  upgrade-typescript-to-5.5 [$] via  v20.12.0 on ☁ïļ  default took 11s
âŊ every-ts bisect bad
The merge base d04e3489b0d8e6bc9a8a9396a633632a5a467328 is bad.
This means the bug has been fixed between d04e3489b0d8e6bc9a8a9396a633632a5a467328 and [27bcd4cb5a98bce46c9cdd749752703ead021a4b].

poweralpha-app/packages/ui on  upgrade-typescript-to-5.5 [$] via  v20.12.0 on ☁ïļ  default took 2s
âŊ every-ts bisect good
The merge base d04e3489b0d8e6bc9a8a9396a633632a5a467328 is bad.
This means the bug has been fixed between d04e3489b0d8e6bc9a8a9396a633632a5a467328 and [27bcd4cb5a98bce46c9cdd749752703ead021a4b d04e3489b0d8e6bc9a8a9396a633632a5a467328].
RyanCavanaugh commented 3 months ago

I've never seen a correct bisect not land on a specific commit. How many steps did it run? You'll also want to make sure you're deleting the .tsbuildinfo between runs, if any

marc-at-brightnight commented 3 months ago

my bad, I read the git docs and now I understand what you guys mean. working through the steps...

marc-at-brightnight commented 3 months ago

ok after 8 steps, some bad, some good, this is what I got:

b3f3bb3d3876d954a6194e6e66ae58e3b74e8c39 is the first bad commit
commit b3f3bb3d3876d954a6194e6e66ae58e3b74e8c39
Author: Wesley Wigham <wewigham@microsoft.com>
Date:   Thu May 23 09:38:57 2024 -0700

    Propagate the error any type in union and intersection construction (#58610)

    Co-authored-by: TypeScript Bot <typescriptbot@microsoft.com>

 src/compiler/checker.ts                                        |  7 +++++--
 src/compiler/types.ts                                          |  4 ++++
 .../jsDeclarationsExportDoubleAssignmentInClosure.types        |  2 +-
 .../reference/jsxNestedWithinTernaryParsesCorrectly.types      |  2 +-
 .../reference/logicalAssignment10(target=es2015).types         |  4 ++--
 .../reference/logicalAssignment10(target=es2020).types         |  4 ++--
 .../reference/logicalAssignment10(target=es2021).types         |  4 ++--
 .../reference/logicalAssignment10(target=esnext).types         |  4 ++--
 .../reference/parsingDeepParenthensizedExpression.types        |  8 ++++----
 .../transpile/declarationSelfReferentialConstraint.d.ts        |  8 ++++++++
 tests/baselines/reference/tsxReactEmitNesting.types            |  2 +-
 .../reference/unionOfFunctionAndSignatureIsCallable.types      | 10 +++++-----
 tests/cases/transpile/declarationSelfReferentialConstraint.ts  |  5 +++++
 13 files changed, 42 insertions(+), 22 deletions(-)
 create mode 100644 tests/baselines/reference/transpile/declarationSelfReferentialConstraint.d.ts
 create mode 100644 tests/cases/transpile/declarationSelfReferentialConstraint.ts
Building TypeScript...
TypeScript built successfully!

poweralpha-app/packages/ui on  upgrade-typescript-to-5.5 [$] via  v20.12.0 on ☁ïļ  default took 3s
âŊ every-ts tsc --noEmit -p .

poweralpha-app/packages/ui on  upgrade-typescript-to-5.5 [$] via  v20.12.0 on ☁ïļ  default took 10s
âŊ every-ts tsc --version
Version 5.5.0-dev

from what I understand 5.5.0-dev is the last "good" commit, and then comes the first bad

weswigham commented 3 months ago

Unless I'm mistaken, there's no functioning repro in this issue? @marc-at-brightnight you said you can only get this to repro in your local project, right? In your local, larger example, is there something //@ts-ignore'd that is maybe affecting this? It'd be an expected change of the diff'd PR if that's the case (previously the error type was treated differently from any, now it's handled like any, and retains its error-ness through union and intersection construction - so this kind of error could appear if, for example, somewhere in the type of the handler is a sneaky error in a dependency that's been suppressed (which could also be suppressed by skipLibCheck)!)

In any case, I don't really have a path forward without a repro. â˜đïļ

marc-at-brightnight commented 3 months ago

@weswigham yeah I figured it would be an issue to not have a reproducible example. There are only a few ts-ignore's in the repo (before I learned about ts-expect-error) but there are mostly in some test files, not related to MUI files.

Shoot! well I was looking forward to upgrading to 5.5 but not sure how to get to a reproducible example. Would have expected others to report this as well but seems like I'm the only one so far... Not sure if/when I'll have time to look at this further but do let me know if you have any more ideas. Maybe we can keep this issue open in the meantime.

eljenso commented 2 weeks ago

Has anyone found a fix or a workaround for this issue?

We are facing the same issue when trying to upgrade from TS v5.4 to 5.6.

jakebailey commented 2 weeks ago

See the comments above; we have no repro so can't look into it. If you have code which shows this exact problem, providing it would be helpful.

eljenso commented 2 weeks ago

I was finally able to reproduce the issue, and the step causing the issue is adding react-three-fiber to the mix. Here is the repro: https://github.com/eljenso/typescript-59474-repro

Once I have added the usage of react-three-fiber, TypeScript started to complain about the types of the event handlers: Image

Weirdly, tsc --noemit doesn't produce any error output.

eljenso commented 1 week ago

@jakebailey Does my repro help you?