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

Treating attribute in JSX as literal type #10393

Closed
yuit opened this issue Aug 17, 2016 · 10 comments
Closed

Treating attribute in JSX as literal type #10393

yuit opened this issue Aug 17, 2016 · 10 comments
Assignees
Labels
Committed The team has roadmapped this issue Domain: JSX/TSX Relates to the JSX parser and emitter Domain: Literal Types Unit types including string literal types, numeric literal types, Boolean literals, null, undefined Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@yuit
Copy link
Contributor

yuit commented Aug 17, 2016

The original issue is issue #10171. Currently we don't treat property assignment of JSX element as literal type as issue #10171 illustrates

type TextProps = { editable: false }
               | { editable: true, onEdit: (newText: string) => void }

class TextComponent extends React.Component<TextProps, {}> {
    render() {
        return <span>Some Text..</span>;
    }
}

ReactDOM.render(
    <TextComponent editable={true} />, // editable is of type boolean not of type "true"
    document.getElementById("example")
);

One possible solution is to treat property as literal type instead. This could possibly affect how overload will work as well

@yuit yuit added Suggestion An idea for TypeScript Domain: JSX/TSX Relates to the JSX parser and emitter Domain: Literal Types Unit types including string literal types, numeric literal types, Boolean literals, null, undefined labels Aug 17, 2016
@yuit yuit added this to the TypeScript 2.1 milestone Aug 17, 2016
@yuit yuit changed the title Treating attribute assignment in JSX as literal type Treating attribute in JSX as literal type Aug 17, 2016
@RyanCavanaugh
Copy link
Member

A quick rundown of problems (or more accurately, reasons this is not a straightforward porting of any existing logic) so we don't forget anything:

  • We need to consider the effect of spread attributes, which have "last in wins" semantics
  • It should be allowed to have 'extra' properties from spreads, but not from explicit attributes
  • Certain kinds of attribute names are categorically ignored for the purposes of extraness (e.g. data- and aria-)

We should also consider the effect of strictNullChecks on spreaded optional properties.

@RyanCavanaugh RyanCavanaugh added the Committed The team has roadmapped this issue label Aug 17, 2016
@RyanCavanaugh
Copy link
Member

Also this work should include handling of overloads on SFCs:

declare function OneThing({x: string}): JSX.Element;
declare function OneThing({y: number}): JSX.Element;
const c1 = <OneThing x='ok' />
const c2 = <OneThing y={42} />
const c3 = <OneThing x='ok' y={10} /> // not OK, y is extraneous

@gcnew
Copy link
Contributor

gcnew commented Aug 17, 2016

I was thinking, is it possible to just desugar the JSX into normal function calls and then let the existing infrastructure handle it? The properties' object type may be passed through a "filter" to remove all aria- and data- props. If you get this machinery working (i.e. the "filtering"), it can also be surfaced as flow does with it's magic functions.

@RyanCavanaugh
Copy link
Member

It isn't, mostly due to spread attributes as noted above.

@gcnew
Copy link
Contributor

gcnew commented Aug 17, 2016

I might be missing something very obvious here, but isn't spread a syntactic sugar for Object.assign (if there are literal attributes as well) which we already have a type for?

@gcnew
Copy link
Contributor

gcnew commented Aug 17, 2016

I guess Object.assign falls short conveying the "last in wins" semantics / extra attributes bits.

@RyanCavanaugh
Copy link
Member

So if you have something like this

interface MyCompProps {
  a: string;
  b: number;
}
const m = { a: 42, b: 15, c: 20 };
const c = <MyCompProps {...m} a={'ok'} />;

You might think "Well, let's just desugar that into..."

const c = <MyCompProps a={42} b={15} c={20} a={'ok'} />;

But we wouldn't let you write this for three different reasons:

  • You can't have duplicate attributes a
  • You can't write a={42} because a is of type string
  • You can't have the undeclared attribute c

You also might write something like this

interface SomeProps {
  a?: string;
}
const s: SomeProps = ...;
const c = <MyCompProps a={42} { ...s } />;

Where it's still invalid to write a={42} because it's just never correct (really you should not write this even if a were not optional in SomeProps).

@gcnew
Copy link
Contributor

gcnew commented Aug 18, 2016

I was thinking of normal functions. With the jsx flavour, jsx elements get transpiled to

React.createElement(comp, props, ...children);

What I have in mind is to transform all elements into this representation beforehand and handle it to the normal typechecker. E.g. in the above case, it would be something like the following:

function __checkSubtype<T extends U, U>(x: T, y: U): U {
    return y;
}

const __tmp = { a: 'ok', d: 3, 'aria-123': 123 };
type __TmpType = $stripAriaAndData(typeof __tmp)
React.createElement(MyCompProps, Object.assign(
    __checkSubtype(
        null  as MyCompProps & Attributes,
        __tmp as __TmpType // cast needed to remove aria
    ),
    m as $stripAriaAndData(typeof m) 
));

The above is not polished, however the key for me is that F-bound polymorphism can be used for excessiveness checking, a type level function for stripping the aria- and data- properties and Object.assign for sticking together the final result (as does the transpiler).

PS: F-Bound polymorphism doesn't properly handle optional properties that are provided. This and data- skipping are currently manifested problems when invoking React.createElement by hand.

@mhegazy mhegazy modified the milestones: TypeScript 2.1, TypeScript 2.1.2, Future Oct 27, 2016
@mhegazy mhegazy modified the milestones: TypeScript 2.2, Future Dec 23, 2016
@yuit
Copy link
Contributor Author

yuit commented Jan 26, 2017

we will have to solve this issue #13526 so the exam will work

@mhegazy mhegazy modified the milestones: TypeScript 2.2, TypeScript 2.3 Feb 2, 2017
@yuit yuit added the Fixed A PR has been merged for this issue label Feb 27, 2017
@yuit
Copy link
Contributor Author

yuit commented Feb 27, 2017

We have take care of this issue when we switch jsx to use overload resolution logic #13640

@yuit yuit closed this as completed Feb 27, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Committed The team has roadmapped this issue Domain: JSX/TSX Relates to the JSX parser and emitter Domain: Literal Types Unit types including string literal types, numeric literal types, Boolean literals, null, undefined Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants