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

feat: add jsx support #391

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

feat: add jsx support #391

wants to merge 2 commits into from

Conversation

erquhart
Copy link

@erquhart erquhart commented Apr 5, 2019

Adds JSX support.

Notes

  • Changed the general test suite to use the esprima dependency rather than 1.0.0-dev
  • Upgraded esprima from 3.1.3 to 4.0.1 for improved JSX support in tests
  • Esprima upgrade is causing tests to fail for two reasons:
    • Esprima outputs string literals with double quotes in 1.0.0, but switched to single quotes in 1.1.0 (source)
    • Esprima no longer represents the presence of a leading 0 in a decimal, so instead of "0.14" it's ".14", both for value and raw
  • Besides the above mentioned issues, tests are passing
  • JSX SpreadChild and Fragments are not supported in this PR, as support for them is not quite widespread (esprima supports neither, I'd consider that to be a decent indicator)

Todo

  • determine if any cases merit throwing an error
  • format output appropriately for compact/non-compact
  • precedence/flag usage review by a maintainer for all uses of generateExpression()

Closes #333.

Copy link
Contributor

@papandreou papandreou left a comment

Choose a reason for hiding this comment

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

I'm no JSX expert, I just wanted to get serialization support in assetgraph. Thanks a lot for picking this up!

I tried npm linking escodegen in with this change, and it seems to do the job! 👍

Would be nice to get support for JSXFragment while you're at it. It's not supported by esprima yet, but you can test that with acorn or espree, for example.

escodegen.js Outdated Show resolved Hide resolved
if (expr.value.type === 'JSXExpressionContainer') {
result.push(this.generateExpression(expr.value.expression, precedence, flags));
return result;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to handle expr.value.type === 'JSXElement' here.

> require('escodegen').generate(require('esprima').parse('(<Foo bar=<Quux/>/>)', {jsx: true}));
'<Foo undefined />;'

And JSXFragment as well, if you go for that: https://github.com/facebook/jsx/blob/master/AST.md#jsx-attributes

Copy link
Author

Choose a reason for hiding this comment

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

Totally forgot about boolean attributes, good catch 👍

Missed it, used to be `[space, '/>]`, forgot to drop the array.

Co-Authored-By: erquhart <[email protected]>
@erquhart
Copy link
Author

Haven't forgotten about this, will circle back soon.

@zouxuoz
Copy link

zouxuoz commented Jan 4, 2020

@erquhart how I can help you to finish this update?

@erquhart
Copy link
Author

erquhart commented Jan 7, 2020

Just a matter of when, haven't circled back yet. An old fork (https://github.com/ng-vu/escodegen-jsx) that introduced (incompatible) jsx support had some things that felt necessary, so the plan has been to pull in the more thorough bits from there. If you want to take this across the finish line, by all means please do!

@sag1v
Copy link

sag1v commented Dec 23, 2021

Is JSX going to be supported or is there any other way to do it?

@smol-honk
Copy link

What if we added this https://github.com/wallabyjs/escodegen?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

this[type] is not a function
5 participants