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

JSX prop with dash is not type checked #32447

Closed
agentcooper opened this issue Jul 17, 2019 · 19 comments
Closed

JSX prop with dash is not type checked #32447

agentcooper opened this issue Jul 17, 2019 · 19 comments
Labels
Add a Flag Any problem can be solved by flags, except for the problem of having too many flags Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@agentcooper
Copy link
Contributor

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 RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jul 17, 2019
@RyanCavanaugh
Copy link
Member

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

@agentcooper
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Collaborator

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

@Pomar81
Copy link

Pomar81 commented Aug 1, 2019

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
Copy link
Member

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

@RyanCavanaugh RyanCavanaugh added the Add a Flag Any problem can be solved by flags, except for the problem of having too many flags label Aug 2, 2019
@RyanCavanaugh
Copy link
Member

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

  • Have another option that controls whether dash-named properties get special treatment
  • Hardcode data- and aria- to be the only allowed special names
  • Only allow dash-named properties for intrinsic elements

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
Copy link

Pomar81 commented Aug 3, 2019

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

@bradleyayers
Copy link

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
Copy link
Contributor

Peeja commented Dec 4, 2019

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
Copy link

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
Copy link

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
Copy link

LEW21 commented Feb 21, 2022

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
Copy link

LeoMcA commented Feb 1, 2023

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?

LeoMcA added a commit to mdn/yari that referenced this issue Feb 2, 2023
as well as data- and any other dashed props

typescript won't alert us to a dashed prop being passed to a component
where it isn't accepted, so it's safest to pass them through wholesale:
microsoft/TypeScript#32447

partial fix to: #8015
caugner pushed a commit to mdn/yari that referenced this issue Feb 2, 2023
pass through aria- props on Button component
as well as data- and any other dashed props

typescript won't alert us to a dashed prop being passed to a component
where it isn't accepted, so it's safest to pass them through wholesale:
microsoft/TypeScript#32447

partial fix to: #8015

* fix: improve aria labels on mobile sidebar button

fixes: #8015

* dashedProps -> passthroughAttrs
@armensargsyan1993
Copy link

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

@MBilalShafi
Copy link

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
Copy link

bvandercar-vt commented Oct 5, 2023

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
Copy link

bvandercar-vt commented Oct 5, 2023

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
Copy link

mattlucock commented Mar 24, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Add a Flag Any problem can be solved by flags, except for the problem of having too many flags Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests