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

Fix: JSXText node trims whitespace too often #328

Merged
merged 5 commits into from
Jul 6, 2024

Conversation

jackschu
Copy link
Contributor

@jackschu jackschu commented Jul 2, 2024

Closes #325 , see that issue for motivation details

I continued to uphold the request in #221 to not generate 'extra' 'jsx_text' nodes, this is not what babel or acorn do so I feel a bit iffy about this.

but completely-whitespace lines are nodes that are stripped by react's jsx_text emitter, so I think it could be OK for there to not be a syntax node here.
(See discussion facebook/react#480 (comment))


I also updated some tests, ie
<Bar> <Foo /> </Bar>

is meant to generate

React.createElement(Bar, null,
    " ",
    React.createElement(Foo, null),
    " ");

So it makes sense that two jsx_text nodes are generated.


Side note: i got rid of the right branch of the choice that said /\/\/[^\n]*/ which seemed to be trying to address would-be-comments. This is not effective ie

<div> // hi </div>

should treat // hi as a jsx_node, but it does not before or after PR (its an ERROR), this seems to be a separate issue

React.createElement("div", null, " // hi ");
<div> // hi  
</div>

Continues to work before and after this PR, but the inner jsx_text node spans 2 lines instead of one. I think this is preferred for tooling to consume because the emitted javascript includes trailing spaces after hi iff theres no newline before the </div>
(See discussion facebook/react#480 (comment))

grammar.js Outdated Show resolved Hide resolved
@amaanq amaanq force-pushed the fix-jsx-text-whitespace branch from 88db75f to fbe1c47 Compare July 6, 2024 00:41
@amaanq
Copy link
Member

amaanq commented Jul 6, 2024

overall looks good - there was one thing I was thinking about before I wanted to merge this, and that was the usage of a tab following a newline vs spaces.

<Foo>
	<Bar />
</Foo>

having a tab before Bar instead of 4 spaces changed the parse tree, because all of \s, \p{Zs}, and some others were not negated in your regex. I've done that now and the trees are consistent, so this looks good to me!! Thank you

(also, you had spurious changes to parser.h/some other files. this is because your cli version is outdated, please use the latest one :) )

@amaanq amaanq merged commit 99be62f into tree-sitter:master Jul 6, 2024
4 checks passed
@jackschu
Copy link
Contributor Author

jackschu commented Jul 6, 2024

Ah thanks for the headsup and thanks for cleaning this up.

@amaanq
Copy link
Member

amaanq commented Jul 6, 2024

thank you very much for the PRs! sorry if I've been slow with reviewing, catching up on a lot 😅

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.

JSXText node trims whitespace too often
2 participants