Skip to content

Use inference for filling JSX attributes in getJsxElementInstanceType#20973

Merged
weswigham merged 2 commits into
microsoft:masterfrom
weswigham:add-inference-for-jsx-target-attributes-type
Jan 3, 2018
Merged

Use inference for filling JSX attributes in getJsxElementInstanceType#20973
weswigham merged 2 commits into
microsoft:masterfrom
weswigham:add-inference-for-jsx-target-attributes-type

Conversation

@weswigham
Copy link
Copy Markdown
Member

Technically, stateless functional components do do an inference pass internally during overload resolution, but checkJsxAttributesAssignableToTagNameAttributes doesn't use those results, since its agnostic to the component kind, but needs to have a correctly instantiated type in all cases so inverted variance positions can be handled correctly in its assignability check.
Fixes #20891

Copy link
Copy Markdown
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a few cleanup items.

@@ -16,11 +20,15 @@ tests/cases/conformance/jsx/file.tsx(17,18): error TS2559: Type '{ a: string; }'

// OK: we fille in missing type argument with empty object
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to update comments here too (or move around the test cases I guess)

internalProp: P;
}

// Error
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update this comment

Comment thread src/compiler/checker.ts Outdated
*
* @param openingLikeElement a non-intrinsic JSXOPeningLikeElement
* @param shouldIncludeAllStatelessAttributesType a boolean indicating whether to include all attributes types from all stateless function signature
* @param sourceAttributesType Is the attributes type the user has actually passed, and is used to create interfences in the target type if present
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete "has actually"
typo "inferences"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As usual, I'm not convinced these @param tags are useful. The @return is, though.

Well, the second sentence for elementType is useful, I guess.

Comment thread src/compiler/checker.ts
// attr1 and attr2 are treated as JSXAttributes attached in the JsxOpeningLikeElement as "attributes".
const sourceAttributesType = createJsxAttributesTypeFromAttributesProperty(openingLikeElement, checkMode);

// targetAttributesType is a type of an attributes from resolving tagName of an opening-like JSX element.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this code move important?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sourceAttributesType is now used in the targetAttributesType assignment expression; it's not just a code move.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed that. Thanks.

Comment thread src/compiler/checker.ts Outdated

function inferJsxTypeArguments(signature: Signature, sourceAttributesType: Type, context: InferenceContext): Type[] {
// Clear out all the inference results from the last time inferTypeArguments was called on this context
for (const inference of context.inferences) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when is a context reused? I only see inferJsxTypeArguments being called right after creating a fresh context.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. It may be possible to reuse them in some way in the future, but I'm not doing so now.

@weswigham weswigham merged commit e0f2033 into microsoft:master Jan 3, 2018
@weswigham weswigham deleted the add-inference-for-jsx-target-attributes-type branch January 3, 2018 19:30
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants