microsoft / TypeScript

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

JSX prop with dash is not type checked #32447

Closed agentcooper closed 5 years ago

agentcooper commented 5 years ago

TypeScript Version: 3.5.1

Search Terms: jsx, dash, props

Code

declare namespace JSX {
  interface ElementAttributesProperty {
    props: any;
  }
}

class MyComponent {
  props: {
    foo?: string;
  }
}

// OK
const el1 = <MyComponent foo="bar" />;

// OK, fooBar is incorrect prop
const el2 = <MyComponent fooBar="bar" />;

// Not OK, foo-bar is incorrect prop, but not reported
const el3 = <MyComponent foo-bar="bar" />;

Expected behavior:

Expected error on el3.

Actual behavior:

No error on el3.

Playground Link

RyanCavanaugh commented 5 years ago

This is the intended behavior because of data- and aria- properties. If we ever get regex-based property names, we'll revisit.

agentcooper commented 5 years ago

@RyanCavanaugh Got it. Does it make sense to have an option to turn off this special case for data- and aria- properties? This is unexpected behavior in non-DOM environments.

RyanCavanaugh commented 5 years ago

If people start running into a lot, we could think about it, but it hasn't seemed to have been an issue so far

typescript-bot commented 5 years ago

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Pomar81 commented 5 years ago

It would be great if we could allow only data- and aria- properties for JSX IntrinsicElements but throw an error in the other cases.

layershifter commented 5 years ago

@RyanCavanaugh I support @agentcooper and @Pomar81 there, it looks like unexpected behavior 💭

RyanCavanaugh commented 5 years ago

I see at least three possible interpretations here and I'm not sure which people are going for:

Is this actually coming up as a problem? Standard props are never named with dashes so it seems like a very difficult mistake to make.

Pomar81 commented 5 years ago

from my side, it would be great if TS Team could apply last option

bradleyayers commented 5 years ago

This came up as a problem for me, when converting HTML to JSX (specifically MJML to mjml-react), and forgetting to convert some of the dashed prop names to camel case. There were no type errors, and essentially no validation on the props that were copied across. I was open to solving this with an ESLint rule, but there appears to be no ESLint rule either.

Peeja commented 4 years ago

Just hit this. We have a custom Button component. We forward a couple of data- attributes to the underlying button. We just tried to add another data- attribute to a Button, expecting it to pass through, but we didn't realize we hadn't forwarded that one yet. It type checked fine, because it was a data- attribute.

I would have expected it not to check the prop on the button, but to check it on the Button.

canonic-epicure commented 3 years ago

This is the intended behavior because of data- and aria- properties. If we ever get regex-based property names, we'll revisit.

@RyanCavanaugh Time to revisit this issue, since template string types appeared?

My use case is that I want to translate all properties like style:${ name } to their corresponding reactive applicators. And now such properties are not typechecked even if props is defined as:

props : { [ key in 'class' | 'style' | `style:${ string }` ]? : string }

So the following declaration is accepted fine (note the typo):

<ProjectPlan styZle:min-width="300px" style="color: red" projectData={ this.launcher.projectData }></ProjectPlan>
pateltejas commented 3 years ago

This also seems to accept propName- as a valid case. We had a typo propName-={} and because of this it didn't catch it. I think it should consider nothing after - as an invalid case. I guess that check shouldn't break any aria related regex

LEW21 commented 2 years ago

Note for other people searching for this:

Right now, it's actually possible to create type-checked prefix:* attributes.

Definition:

interface Attributes {
    [key: `class:${string}`]: boolean;
}

Usage:

<div class:whatever={true}>
<div class:whatever={false}>
<div class:whatever> <!-- evaluates to true -->
LeoMcA commented 1 year ago

I believe we just hit this on MDN too: a user reported no aria-label on one of our buttons which had been set on our component, but on the aria-label prop, whereas this component required an ariaLabel prop.

Due to this behaviour in TypeScript, we got absolutely no indication of the prop naming mismatch, which is surely the point of typing our component props. Could this bug be reopened?

armensargsyan1993 commented 1 year ago

Hi, are we have news about it? This is creating some problems for me.

MBilalShafi commented 1 year ago

I'd say it could be useful to type restrict or control which hyphen-cased props are allowed, because sometimes in the component's internal logic, we are not forwarding { ...rest } to the DOM element, and only forward a subset of props, which gives an incorrect notion that a certain prop is supported (or passed down to the dom element) which is not necessarily the use-case.

A similar scenario occurred in MUI DataGrid when we a user tried to pass data-* attributes which were actually not forwarded but the TS didn't complain it.

A similar use-case is also mentioned by @Peeja above.

bvandercar-vt commented 1 year ago

I'd say it could be useful to type restrict or control which hyphen-cased props are allowed, because sometimes in the component's internal logic, we are not forwarding { ...rest } to the DOM element, and only forward a subset of props, which gives an incorrect notion that a certain prop is supported (or passed down to the dom element) which is not necessarily the use-case.

I think this is the biggest reason that this bug should be fixed. There is no guarantee that a component is spreading these props, so why always allow them? If a component did want to allow these props, they could always manually extend HtmlProps or AriaAttributes to their component's props. But they should only be allowed if these are extended.

bvandercar-vt commented 1 year ago

Another reason: Let's say you are spreading { ...rest } to a component, but overriding a prop, i.e.

<div
   {...rest}
   aria-label="label"

You'd want to have the component's props be something like Omit<HtmlProps<HtmlDivElement>, 'aria-label'> , to disallow aria-label from being passed, since it is overwritten. However, this currently does not work, as aria-label is always allowed to be passed.

mattlucock commented 7 months ago

I am working on a library with a JSX API, in which hyphenated prop names are explicitly not allowed. Given the context of the library, however, my users may inadvertently write them anyway. Not only will the behavior they expect not occur if they do this, but in fact, in the case of my library, behavior that is actually erroneous will occur if they do this. As such, it's imperative that I enforce types that prevent them from doing this.

But apparently, this is impossible. No matter how strictly I author my types, the type checker will always allow my users to specify props that I know are wrong. I am frustrated to learn that this is another case in which the type checker deliberately defeats its own safety. To be honest, from reading the documentation on typing JSX (which lead me to discover this issue), most of the type-checking behavior for JSX feels unsound. Whereas most of TypeScript's general type-checking behavior feels sound, the JSX behavior just feels like a series of special cases (such as this) stuck together.

As it happens, the JSX API is actually optional in my library. It seems to me that I should warn my users against using it, since it turns out that JSX is actually not type safe. Perhaps I shouldn't even support it at all.

std4453 commented 3 months ago

Note for other people searching for this:

Right now, it's actually possible to create type-checked prefix:* attributes.

Definition:

interface Attributes {
    [key: `class:${string}`]: boolean;
}

Usage:

<div class:whatever={true}>
<div class:whatever={false}>
<div class:whatever> <!-- evaluates to true -->

One of our projects tried this to explicitly define data-* props, and it turns out to be problematic since that would pressure TS type checker to its extreme when other type operations are involved, resulting in TS errors like:

Expression produces a union type that is too complex to represent. ts(2590)