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

Support for --jsxFactory option #12135

Merged
merged 8 commits into from
Nov 10, 2016
Merged

Support for --jsxFactory option #12135

merged 8 commits into from
Nov 10, 2016

Conversation

sheetalkamat
Copy link
Member

Fixes #9582 and #12067

@@ -2250,6 +2251,7 @@ namespace ts {
/* @internal */
export interface TypeCheckerHost {
getCompilerOptions(): CompilerOptions;
getJsxFactoryEntity(): EntityName;
Copy link
Contributor

Choose a reason for hiding this comment

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

this feels odd. i would rather we parsed the jsxFactory string one more time in the checker instead.

@sheetalkamat sheetalkamat merged commit c458576 into master Nov 10, 2016
@sheetalkamat sheetalkamat deleted the jsxFactory branch November 10, 2016 17:50
This was referenced Nov 10, 2016
@appsforartists
Copy link

Do we still need typescript@next for this? https://github.com/Microsoft/TypeScript/wiki/Roadmap says it's landed, but I'm seeing

error TS5023: Unknown compiler option 'jsxFactory'.

in 2.1.1.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 29, 2016

The roadmap says it is in typescript@next. the issues (#9582 and #12067) are marked with milestone TypeScript 2.1.3 which is not TypeScript 2.1.1 and has not shipped yet.

@appsforartists
Copy link

Thanks for the clarification.

I'm not sure the roadmap makes that clear. The heading "2.1 (November 2016)" reads like those features have already shipped in 2.1.0, especially since they have little checkmarks next to them. Someone would have to scroll all the way to the bottom to realize the checkmarks are meant to connote "next" features.

Moreover, the PR linked from the Roadmap doesn't have a version label/milestone attached to it.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 29, 2016

At the bottom of the roadmap page, there is an explanation that the checkmark means it is available in typescript@next.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 29, 2016

did not mean to sound pedantic. just wanted to make sure the process is clear. questions are always welcomed.

@appsforartists
Copy link

I saw that, but only after your first response. It never occurred to me to scroll to the bottom of the page until you said "The roadmap says it is in typescript@next".

Have you considered putting the checkbox descriptor at the top? How about making your roadmap more granular (e.g. change the Nov heading to 2.1.1: stable and have a heading above it for next)?

I don't mean to be nit picky, but I'm sure I'm not the last person who will misread the current Roadmap.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 29, 2016

thanks for the tip. moved the note to the top. and will consider having better naming for milestones in the future.

@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants