GeezerVison

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

GeezerVison is showing github.com — Add support for transpiling per-file jsx pragmas by weswigham · Pull Request #21218 · microsoft/TypeScript

Conversation

Wesley Wigham (weswigham) commented Jan 16, 2018 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

This adds support for per-file jsx factory pragmas (similar to babel):

/** @jsx dom */
import { dom } from "./renderer"
<h></h>

becomes

var renderer_1 = require("./renderer");
renderer_1.dom("h", null);

We still expect the global jsx compiler option to be set and the file extension to be tsx or jsx; but now you can mix multiple jsx frameworks in one project (just not more than one per file). This only affects emit and not typechecking - multiple different frameworks' types attempting to define, eg, JSX.Element can still result in type errors if those definitions conflict.

Fixes #21114, probably?
ref #18131

Sorry, something went wrong.

Uh oh!

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

All reactions


          Add support for per-file jsx pragmas

f4d3143

Wesley Wigham (weswigham) requested review from a user, Ryan Cavanaugh (RyanCavanaugh), Mohamed Hegazy (mhegazy) and Ben Lichtman (uniqueiniquity)

January 16, 2018 23:50

Ryan Cavanaugh (RyanCavanaugh) approved these changes

Jan 17, 2018

View reviewed changes

Ben Lichtman (uniqueiniquity) suggested changes

Jan 18, 2018

View reviewed changes

Ben Lichtman (uniqueiniquity) left a comment

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.

Currently, JSX Fragments emit an error when used when jsxFactory is defined in the compiler options. This may change in the future, but until then you might need additional changes to ensure the error happens when the factory is only defined locally.

Sorry, something went wrong.

Uh oh!

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

All reactions


          Add error for using jsx factory pragma with fragments

52a6f22

Wesley Wigham (weswigham) force-pushed the multiple-jsx-factories branch from 28a1178 to 52a6f22 Compare

January 18, 2018 23:27

Wesley Wigham (weswigham) commented Jan 18, 2018

Copy link

Copy Markdown

Member Author

Ben Lichtman (@uniqueiniquity) A similar error is now present when a fragment is used alongside an inline jsx pragma. 🌞

All reactions

Sorry, something went wrong.

Uh oh!

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

Ben Lichtman (uniqueiniquity) approved these changes

Jan 19, 2018

View reviewed changes

Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request

Feb 9, 2018

Design Meeting Notes, 2/9/2018 #21838

Closed


          Merge branch 'master' into multiple-jsx-factories

fa36640

Wesley Wigham (weswigham) commented Feb 22, 2018

Copy link

Copy Markdown

Member Author

Mohamed Hegazy (@mhegazy) Do you wanna give this a brief look-over before it's merged?

All reactions

Sorry, something went wrong.

Uh oh!

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

Mohamed Hegazy (mhegazy) reviewed

Feb 22, 2018

View reviewed changes

Comment thread

tests/cases/conformance/jsx/inline/inlineJsxFactoryWithFragmentIsError.tsx

		}
		export function dom(): void;
		// @filename: index.tsx
		/** @jsx dom */

Mohamed Hegazy (mhegazy) Feb 22, 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.

add a test with ** @jsx React.createElement */

Sorry, something went wrong.

Uh oh!

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

weswigham reacted with thumbs up emoji

All reactions

👍 1 reaction

Mohamed Hegazy (mhegazy) reviewed

Feb 22, 2018

View reviewed changes

Comment thread

src/compiler/parser.ts Outdated

		const referencedFiles: FileReference[] = [];
		const typeReferenceDirectives: FileReference[] = [];
		const amdDependencies: { path: string; name: string }[] = [];
		let pragmas: { pragma: string, value: string \| undefined }[] = [];

Mohamed Hegazy (mhegazy) Feb 22, 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.

if we gonna add a new concept, then we should consolidate the other ones under pargma.. and we should allow /// as well as multi-line comments.

I would also have helper functions for getting all of these like amdDependecies, amdModuleNAme, checkJs, etc..

Sorry, something went wrong.

Uh oh!

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

All reactions

Wesley Wigham (weswigham) Feb 23, 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.

Sure! I wanted to consolidate them, anyway, just didn't think this PR was appropriate.

Sorry, something went wrong.

Uh oh!

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

All reactions

Sheetal Nandi (sheetalkamat) Feb 23, 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.

We need to ensure this works well when doing incremental parsing as well and need test for the same.

Sorry, something went wrong.

Uh oh!

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

All reactions

Mohamed Hegazy (mhegazy) reviewed

Feb 22, 2018

View reviewed changes

Comment thread

src/compiler/checker.ts

		if (file.localJsxNamespace) {
		return file.localJsxNamespace;
		}
		const jsxPragma = file.pragmas.get("jsx");

Mohamed Hegazy (mhegazy) Feb 22, 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.

why is it jsx and not jsxNamespace like the compiler option?

Sorry, something went wrong.

Uh oh!

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

All reactions

Wesley Wigham (weswigham) Feb 23, 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.

Babel supports jsx and I figured we'd want to support the same thing - we can alias jsxNamespace, if you'd like?

Sorry, something went wrong.

Uh oh!

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

All reactions

Mohamed Hegazy (mhegazy) reviewed

Feb 22, 2018

View reviewed changes

Comment thread

tests/cases/conformance/jsx/inline/inlineJsxFactoryWithFragmentIsError.tsx

		}
		}
		export function dom(): void;
		// @filename: index.tsx

Mohamed Hegazy (mhegazy) Feb 22, 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.

also add a test with two files, and @jsxNamespace defined, and one of them with the new pragma.

Sorry, something went wrong.

Uh oh!

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

weswigham reacted with thumbs up emoji

All reactions

👍 1 reaction


          More tests, use different regex class for pragma capture

3abda0a

Wesley Wigham (weswigham) force-pushed the multiple-jsx-factories branch from 7a7f20e to 3abda0a Compare

February 24, 2018 03:50


          Unify all pragma parsing machinery

8d5c733

Wesley Wigham (weswigham) commented Feb 24, 2018 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member Author

Mohamed Hegazy (@mhegazy) Sheetal Nandi (@sheetalkamat) I've unified all pragma-parsing infrastructure such that new pragmas can be added just by editing an object literal, alternate syntaxes enabled, etc - also this PR is much larger now. (And a bit off topic, which is why I wanted to do it later, but w/e, it's here now)

Mohamed Hegazy (@mhegazy) Which old/new pragmas did you want to allow under which syntaxes (XML-like triple slash, singleline @-pragma or multiline @-pragma)? (So I can update their config and add tests of the alternate forms) I imagine we can be permissive and any ones we don't consider legacy we could allow under all the syntaxes we like, so users don't have to remember which syntax we actually parse.

Sheetal Nandi (@sheetalkamat) I think now that I've unified the implementations, incremental is handled correctly - preProcess.ts is the implementation of re-scanning this data for incremental purposes, right? Now it handles the new pragmas the same way as the old ones (and the same way as the parser, for that matter).

All reactions

Sorry, something went wrong.

Uh oh!

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

Wesley Wigham (weswigham) requested a review from Nathan Shively-Sanders (sandersn)

February 26, 2018 21:30

Mohamed Hegazy (mhegazy) reviewed

Feb 26, 2018

View reviewed changes

Comment thread

src/compiler/checker.ts

		}
		const jsxPragma = file.pragmas.get("jsx");
		if (jsxPragma) {
		const chosenpragma = isArray(jsxPragma) ? jsxPragma[0] : jsxPragma;

Mohamed Hegazy (mhegazy) Feb 26, 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.

why not set this one in processPragmasIntoFields like the other ones, just for consistency.

Sorry, something went wrong.

Uh oh!

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

All reactions

Wesley Wigham (weswigham) Feb 26, 2018 •

edited

Loading

Uh oh!

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

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.

It pretty much maps to localJsxFactory, its calculation is just deferred.

Sorry, something went wrong.

Uh oh!

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

All reactions

Wesley Wigham (weswigham) Feb 26, 2018 •

edited

Loading

Uh oh!

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

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.

I could move the calculation (and make it eager) from the checker into the parser, if you'd like.

Sorry, something went wrong.

Uh oh!

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

All reactions

Mohamed Hegazy (mhegazy) approved these changes

Feb 26, 2018

View reviewed changes

Wesley Wigham (weswigham) merged commit 32c63a2 into microsoft:master

Feb 27, 2018

Wesley Wigham (weswigham) deleted the multiple-jsx-factories branch

February 27, 2018 00:10

Mohamed Hegazy (mhegazy) mentioned this pull request

Feb 27, 2018

Lookup JSX namespace within factory function #22207

Merged

Microsoft (microsoft) locked and limited conversation to collaborators

Jul 3, 2018

Reviewers

Sheetal Nandi (sheetalkamat)

sheetalkamat left review comments

Ryan Cavanaugh (RyanCavanaugh)

RyanCavanaugh approved these changes

Nathan Shively-Sanders (sandersn)

Awaiting requested review from sandersn

+2 more reviewers

Mohamed Hegazy (mhegazy)

mhegazy approved these changes

Ben Lichtman (uniqueiniquity)

uniqueiniquity 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

None yet

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.

5 participants

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.

Conversation

Wesley Wigham (weswigham) commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ben Lichtman (uniqueiniquity) left a comment

Choose a reason for hiding this comment

Uh oh!

Wesley Wigham (weswigham) commented Jan 18, 2018

Uh oh!

Wesley Wigham (weswigham) commented Feb 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wesley Wigham (weswigham) commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wesley Wigham (weswigham) Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wesley Wigham (weswigham) Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Wesley Wigham (weswigham) commented Jan 16, 2018 •

edited

Loading

Uh oh!

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

Wesley Wigham (weswigham) commented Feb 24, 2018 •

edited

Loading

Uh oh!

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

Wesley Wigham (weswigham) Feb 26, 2018 •

edited

Loading

Uh oh!

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

Wesley Wigham (weswigham) Feb 26, 2018 •

edited

Loading

Uh oh!

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