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

XJS -> JSX #163

Merged
merged 1 commit into from
Mar 3, 2015
Merged

XJS -> JSX #163

merged 1 commit into from
Mar 3, 2015

Conversation

fkling
Copy link
Contributor

@fkling fkling commented Mar 2, 2015

Depends on benjamn/ast-types#93. I did a simple search/replace here as well and ran npm test with the updated ast-types version.

@fkling
Copy link
Contributor Author

fkling commented Mar 2, 2015

esprima-fb has to be bumped too (tests still pass).

@benjamn
Copy link
Owner

benjamn commented Mar 2, 2015

For backwards compatibility, can we keep the old case labels for now?

case "XJSElement":
case "JSXElement":
    var openingLines = path.call(print, "openingElement");
    ...

@fkling
Copy link
Contributor Author

fkling commented Mar 2, 2015

Yeah, I wanted to ask you about that and forgot... will update.

@fkling
Copy link
Contributor Author

fkling commented Mar 2, 2015

Although, is this actually necessary if ast-types cannot produce XJS* nodes anymore? Anyways, I updated it.

@benjamn
Copy link
Owner

benjamn commented Mar 2, 2015

Yeah, older versions of esprima-fb can still produce XJS* nodes, so I think we should be able to print them.

@fkling
Copy link
Contributor Author

fkling commented Mar 2, 2015

Maybe I'm missing something, but wouldn't recast not be able to handle "older" ASTs because the new version of ast-types doesn't support XJS* nodes either? Or are you planning to merge this change independently of the ast-types version?

Or do some parts of recast work without ast-types and directly on the AST?

benjamn added a commit that referenced this pull request Mar 3, 2015
@benjamn benjamn merged commit 3c12c3b into benjamn:master Mar 3, 2015
@benjamn
Copy link
Owner

benjamn commented Mar 3, 2015

Do you know if parentheses are still needed around multi-line JSX return arguments?

@fkling
Copy link
Contributor Author

fkling commented Mar 3, 2015

You mean for

return (
    <div>
    // ...
   </div>
);

?

I would believe so. If there is a line break after the return, ASI will kick in. A JSX -> JS transpiler (or even recast?) might be able to detect that and move the opening tag on the same line, but that seems more magical.

@jenseng
Copy link
Contributor

jenseng commented Mar 5, 2015

parens shouldn't be needed, since tag balancing trumps ASI... for example, jsx cli (react-tools) converts this:

return <div>
  there are
  <b>multiple</b> lines
  here
</div>;

into this:

return React.createElement("div", null,
  "there are",
  React.createElement("b", null, "multiple"), " lines" + ' ' +
  "here"
);

@fkling
Copy link
Contributor Author

fkling commented Mar 5, 2015

Have parens be ever needed for

return <div>
  there are
  <b>multiple</b> lines
  here
</div>;

?

@fkling fkling deleted the jsx branch March 5, 2015 04:33
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.

None yet

3 participants