Skip to content
This repository has been archived by the owner on Jan 19, 2019. It is now read-only.

New: Implements JSX syntax (fixes #18) #28

Merged
merged 1 commit into from
Mar 18, 2016
Merged

New: Implements JSX syntax (fixes #18) #28

merged 1 commit into from
Mar 18, 2016

Conversation

JamesHenry
Copy link
Member

@nzakas would be great to get your feedback on this!

Note: I added lodash.unescape as a dependency to ensure that HTML entities in JSX tags are decoded appropriately. Now that lodash is entirely modular, I felt that it was more prudent to include a micro-dependency than reinvent the wheel.

@eslintbot
Copy link

LGTM

"jsx/namespaced-attribute-and-value-inserted", // https://github.com/Microsoft/TypeScript/issues/7411
"jsx/namespaced-name-and-attribute", // https://github.com/Microsoft/TypeScript/issues/7411
"jsx/test-content", // https://github.com/Microsoft/TypeScript/issues/7471
"jsx/multiple-blank-spaces"
Copy link
Member Author

Choose a reason for hiding this comment

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

I have not had chance to report this one yet (jsx/multiple-blank-spaces), but might be another bug with some internal TypeScript utilities. The body of the AST is correct, but we cannot currently populate tokens array properly because of the issue.

@nzakas
Copy link
Member

nzakas commented Mar 14, 2016

This looks really good. The only thing I think is missing is that it appears the parser will always parse JSX syntax. We may want to ensure that parse() only parses JSX when ecmaFeatures.jsx is true (as I'm sure there are people using TypeScript without JSX).

@JamesHenry
Copy link
Member Author

Sorry if I am missing something obvious, but wouldn't the changes I made in parser.js (https://github.com/eslint/typescript-eslint-parser/pull/28/files#diff-09461573a85e2d94f056dd6814769042R112) achieve exactly that?

You can try creating and running an index.js in the root of the project with the following content as a quick POC:

var code = '<div>@test content</div>';
var parser = require('./parser').parse
console.log(parser(code, {ecmaFeatures: {jsx: true}}))

If ecmaFeatures.jsx is not truthy, it will throw.

Again, if I am missing something else, please let me know and I will fix it!

@nzakas
Copy link
Member

nzakas commented Mar 15, 2016

All that ends up doing is changing the filename passed at https://github.com/eslint/typescript-eslint-parser/pull/28/files#diff-09461573a85e2d94f056dd6814769042R117

I'm not sure changing that filename is the same as enabling/disabling JSX support. Are you?

@JamesHenry
Copy link
Member Author

But... https://github.com/eslint/typescript-eslint-parser/pull/28/files#diff-09461573a85e2d94f056dd6814769042R154? :)

As well using it to determine that filename, we are using extra.ecmaFeatures.jsx to inform the setting of the jsx option on the TypeScript compiler, and as we know from: https://github.com/Microsoft/TypeScript/wiki/JSX#basic-usage

In order to use JSX you must do two things.

  • Name your files with the .tsx extension
  • Enable the jsx option

...so that is both aspects covered, right?

@nzakas
Copy link
Member

nzakas commented Mar 18, 2016

Oh, oops! Sorry, my Lyme brain must be getting to me. This looks good!

nzakas added a commit that referenced this pull request Mar 18, 2016
New: Implements JSX syntax (fixes #18)
@nzakas nzakas merged commit a18683b into eslint:master Mar 18, 2016
@nzakas
Copy link
Member

nzakas commented Mar 18, 2016

I just pushed a new version with this fix.

@JamesHenry JamesHenry deleted the tsx branch August 22, 2016 20:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants