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

Add support for transpiling per-file jsx pragmas #21218

Merged
merged 5 commits into from
Feb 27, 2018

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Jan 16, 2018

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

Copy link
Contributor

@uniqueiniquity uniqueiniquity left a 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.

@weswigham weswigham force-pushed the multiple-jsx-factories branch from 28a1178 to 52a6f22 Compare January 18, 2018 23:27
@weswigham
Copy link
Member Author

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

@weswigham
Copy link
Member Author

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

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

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 */

@@ -6089,6 +6089,7 @@ namespace ts {
const referencedFiles: FileReference[] = [];
const typeReferenceDirectives: FileReference[] = [];
const amdDependencies: { path: string; name: string }[] = [];
let pragmas: { pragma: string, value: string | undefined }[] = [];
Copy link
Contributor

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..

Copy link
Member Author

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.

Copy link
Member

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.

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

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?

Copy link
Member Author

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?

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

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.

@weswigham weswigham force-pushed the multiple-jsx-factories branch from 7a7f20e to 3abda0a Compare February 24, 2018 03:50
@weswigham
Copy link
Member Author

weswigham commented Feb 24, 2018

@mhegazy @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)

@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.

@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).

@weswigham weswigham requested a review from sandersn February 26, 2018 21:30
}
const jsxPragma = file.pragmas.get("jsx");
if (jsxPragma) {
const chosenpragma = isArray(jsxPragma) ? jsxPragma[0] : jsxPragma;
Copy link
Contributor

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.

Copy link
Member Author

@weswigham weswigham Feb 26, 2018

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.

Copy link
Member Author

@weswigham weswigham Feb 26, 2018

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.

@weswigham weswigham merged commit 32c63a2 into microsoft:master Feb 27, 2018
@weswigham weswigham deleted the multiple-jsx-factories branch February 27, 2018 00:10
@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.

5 participants