Skip to content

No validation in JSX when using template string index signatures #44797

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

Open
DanielRosenwasser opened this issue Jun 28, 2021 · 11 comments
Open
Assignees
Labels
In Discussion Not yet reached consensus Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 28, 2021

Playground Link

Here's the shortest example:

import * as React from "react";

interface Props {
    foo: string;
    bar: number;
    [dataProp: `data-${string}`]: string;
}

function Yadda(props: Props) {
    return <div />;
}

let props: Props = {
    foo: "",
    bar: 0,
    "data-yadda": 42,
};

let x = <Yadda foo="hello" bar={42} data-yadda={42} />;

Expected: TypeScript should error on data-yadda, just as if we had explicitly declared a data-yadda property in Props, or if we had used a string index signature.
Actual: JSX dashed properties are exempt from these checks on the target object if a corresponding property doesn't exist.

My expectation is that if an index signature exists, and a property should be covered by that index signature, then TypeScript should check that the property is compatible with that index signature.

Here's a longer example that could be used in tests.

import * as React from "react";

interface Props {
    foo: string;
    bar: number;
    [dataProp: `data-${string}`]: string;
}

function Yadda(props: Props) {
    return <div />;
}

// Errors as expected on data-yadda. ✅
let propsObj1: Props = {
    foo: "",
    bar: 0,
    "data-yadda": 42,
};

// Should error on data-yadda, does not. ❌
let x = <Yadda foo="hello" bar={42} data-yadda={42} />;

/////////

let propsObj2 = {
    foo: "",
    bar: 0,
    "data-yadda": 42,
};

// Should error on data-yadda, does not. ❌
let y = <Yadda {...propsObj2} />

// Errors as expected on data-yadda. ✅
propsObj1 = propsObj2;
@DanielRosenwasser
Copy link
Member Author

@dzearing @JasonGore

@DanielRosenwasser
Copy link
Member Author

I think that since string index signatures actually do cover these properties, this is just an oversight in implementation and can be considered a bug.

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Jun 30, 2021

Okay, this is bananas.

import * as React from "react";

interface PropsA {
    foo: string;
    [dataProp: string]: string;
}

function ComponentA(props: PropsA) {
    return <div />;
}

// Error on data-yadda ✅
let x = <ComponentA foobar="hello" data-yadda={42} />;

/////////////////////

interface PropsB {
    foo: string;
    [dataProp: string]: string;
}

function ComponentB(props: PropsB) {
    return <div />;
}

// No error ❌ 😲
let y = <ComponentB foo="hello" data-yadda={42} />;

@ahejlsberg
Copy link
Member

The bananas behavior is because we ignore JSX attributes with dashed names in relationship checking, but then fail to ignore them in error elaboration. That's a pretty simple fix.

@ahejlsberg
Copy link
Member

I think that since string index signatures actually do cover these properties, this is just an oversight in implementation and can be considered a bug.

No, string index signatures actually don't cover dashed names today. They only do in the broken error elaboration logic because it fails to ignore the dashed names.

It would definitely be a breaking change to always validate dashed names against string index signatures. Best non-breaking change we could consider is to not validate dashed names against string index signatures, but to validate them against template literal index signatures. But I'm not sure I could ever explain the rationale behind that with a straight face.

@DanielRosenwasser
Copy link
Member Author

But I'm not sure I could ever explain the rationale behind that with a straight face.

We're going to have to do that with #44815 anyway. 😅

@ahejlsberg
Copy link
Member

Fix for the strange error elaboration behavior in #44873. Still not sold on validation against template string index signatures.

@ahejlsberg ahejlsberg added In Discussion Not yet reached consensus Suggestion An idea for TypeScript and removed Bug A bug in TypeScript labels Jul 2, 2021
@DanielRosenwasser
Copy link
Member Author

It's worth mentioning that we can always do the checking later and loosen it after. Maybe @dzearing can give some context on the use-case that Fluent UI had in mind.

@andrewbranch andrewbranch added the Rescheduled This issue was previously scheduled to an earlier milestone label Aug 30, 2021
@DanielRosenwasser
Copy link
Member Author

I've filed #46229 as an alternative that allows the break to be opt-in for all index signatures; however, I think there's a lot of value in just removing the exemption for all non-string and non-number index signatures because we're just opening ourselves to more long-term unsafety.

@matthewp
Copy link

It this the same bug I'm running into? I'm using dashes in index signatures for the tag name (IntrinsicElements), not in attributes. I think it might have the same underlying cause, so I'm bringing it up so that a solution takes this into account and not just attributes.

https://www.typescriptlang.org/play?target=99&jsx=4#code/CYUwxgNghgTiAEBzCB7ARlC8DeAoe8AdlALYgDOADlGAgFIDKAGjvgfCAB6UowAu8AJaE+IGADMaCAJIiYw8oLABRCCDIjyrduwD0u+ABUAFoK3AUFIigEB3XgGs2OgNoOAXPAAGAEmzk+eUJEAF8AWj8AoNCvAF1PKEIATwBuXGc9AxMzeHsYB3IMgn14N09ff0DhGPj4RNSMkLYmptwwFEIA+AsSeABeeAAeYxAIVDC8iGAAPkHdEbGUCd4p6fT2zpQ1ADpURAAKHoBKFKA

@RyanCavanaugh RyanCavanaugh removed this from the TypeScript 4.8.0 milestone Feb 1, 2023
@Andarist
Copy link
Contributor

Andarist commented Apr 2, 2024

@matthewp your issue was fixed by #55245

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants