GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Lookup JSX namespace within factory function by weswigham · Pull Request #22207 · microsoft/TypeScript

Conversation

@weswigham

@weswigham Wesley Wigham (weswigham) commented Feb 27, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This allows you to mix the types for multiple jsx factories in one compilation in a similar way to how you can now mix factory functions. What this change does is look for an exported JSX namespace under the "react namespace" of the factory function. What this means is that if the factory function is React.createElement, we'll look for React.JSX. If the factory function is p, we'll look for p.JSX. If a local JSX namespace cannot be found, we still fallback to the global one, if present. This does not change how we discover any JSX types beyond looking for them in an additional location.

An an example, react's core exports can now be defined like so:

export function createElement<P>(/*complex types*/): Component<P>;
export namespace createElement {
    export namespace JSX {
        interface Element extends ReactElement<any> {}
        interface ClassElement extends Component<any> {}
        // ... and so on
    }
}

and you get everything you need on the consuming end with your standard

import * as React from "react";
export const MyComponent = (p: {x?: number, y?: number}) => <h1>/*...*/</h1>;

With this change, a JSX package no longer needs to pollute the global scope with types that can conflict with other packages. 😄

Fixes #18131
IMHO, this elegantly solves the aliasing problem posed in the original issue (we're just doing the lookup inside the factory function's namespace, rather than trying to trace back into the file it came from and looking beside it)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@weswigham
Wesley Wigham (weswigham) requested review from Daniel Rosenwasser (DanielRosenwasser), Ryan Cavanaugh (RyanCavanaugh) and Mohamed Hegazy (mhegazy) February 27, 2018 19:11
@weswigham
Lookup JSX namespace within factory function
488397f
@weswigham
Wesley Wigham (weswigham) force-pushed the local-jsx-namespaces branch from d812784 to 488397f Compare February 27, 2018 19:21
@mhegazy

Mohamed Hegazy (mhegazy) commented Feb 27, 2018

Copy link
Copy Markdown
Contributor

Wesley Wigham (@weswigham) can you share perf numbers on one of our RWC jsx code bases comparing to before #21218, to #21218 + this change.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@weswigham

Wesley Wigham (weswigham) commented Feb 28, 2018

Copy link
Copy Markdown
Member Author

image
👍 👎?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

RyanCavanaugh
Ryan Cavanaugh (RyanCavanaugh) reviewed Feb 28, 2018
View reviewed changes
Comment thread src/compiler/checker.ts Outdated
return getUnionType(map(instantiatedSignatures, getReturnTypeOfSignature), UnionReduction.Subtype);
}

function getJsxNamespaceForLocation(location: Node) {

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) Feb 28, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Super nit, the naming of getJsxNamespaceForLocation should probably match getJsxStatelessElementTypeAt / getJsxElementClassTypeAt

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
RyanCavanaugh
Ryan Cavanaugh (RyanCavanaugh) reviewed Feb 28, 2018
View reviewed changes
Comment thread src/compiler/checker.ts Outdated
*/
function getJsxIntrinsicTagNames(): Symbol[] {
const intrinsics = getJsxType(JsxNames.IntrinsicElements);
function getJsxIntrinsicTagNames(location: Node): Symbol[] {

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) Feb 28, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Similar, getJsxIntrinsicTagNamesAt

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@RyanCavanaugh

Ryan Cavanaugh (RyanCavanaugh) commented Feb 28, 2018

Copy link
Copy Markdown
Member

Very nice

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

RyanCavanaugh
Ryan Cavanaugh (RyanCavanaugh) approved these changes Feb 28, 2018
View reviewed changes
@weswigham
Rename functions
13fa1a8
mhegazy
Mohamed Hegazy (mhegazy) reviewed Feb 28, 2018
View reviewed changes
Comment thread tests/baselines/reference/api/tsserverlibrary.d.ts
getAliasedSymbol(symbol: Symbol): Symbol;
getExportsOfModule(moduleSymbol: Symbol): Symbol[];
getAllAttributesTypeFromJsxOpeningLikeElement(elementNode: JsxOpeningLikeElement): Type | undefined;
getJsxIntrinsicTagNames(): Symbol[];

@mhegazy Mohamed Hegazy (mhegazy) Feb 28, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

please add a note about this change in the breaking changes section in https://github.com/Microsoft/TypeScript/wiki/API-Breaking-Changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@weswigham Wesley Wigham (weswigham) Feb 28, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done. 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
mhegazy
Mohamed Hegazy (mhegazy) approved these changes Feb 28, 2018
View reviewed changes
@mhegazy Mohamed Hegazy (mhegazy) added Breaking Change Would introduce errors in existing code Domain: API Relates to the public API for TypeScript labels Feb 28, 2018
@weswigham
Wesley Wigham (weswigham) merged commit 1a43ad0 into microsoft:master Feb 28, 2018
@weswigham
Wesley Wigham (weswigham) deleted the local-jsx-namespaces branch February 28, 2018 23:48
@weswigham Wesley Wigham (weswigham) mentioned this pull request Mar 8, 2018
Using lookahead to detect generic JSX elements? #6395
Closed
@microsoft 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.

Reviewers

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) RyanCavanaugh approved these changes
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Awaiting requested review from DanielRosenwasser
+1 more reviewer
@mhegazy Mohamed Hegazy (mhegazy) mhegazy approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Breaking Change Would introduce errors in existing code Domain: API Relates to the public API for TypeScript

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@weswigham @mhegazy @RyanCavanaugh
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.