Various fixes for JSX support

guillaumebrunerie commented 1 year ago

I made some changes to JSX parsing:

Removed the special treatment of JSX fragments in the grammar, instead the name and attributes of jsx_opening_element and the name of jsx_closing_element are now optional. This decreases the state count from 1584 to 1533 and simplifies highlighting rules and similar by not requiring special cases for JSX fragments.
Trim whitespace before and after, and split at new lines in jsx_text (see #221)
Made </ and /> single tokens instead of two, makes more sense to me.
Made the Foo.Bar part in <Foo.Bar/> have the same parse tree as in const Component = Foo.Bar using aliases. We get the parse tree (member_expression (identifier) (property_identifier)) instead of (nested_identifier (identifier) (identifier))
Highlight JSX brackets as brackets
Only highlight JSX tags as tags if they don’t contain periods and start with a lowercase letter (because <div/> and <Div/> are fundamentally different, the first one refers to the HTML element named "div" while the second one refers to the component Div which is a defined identifier in scope)

I have a corresponding change for the Typescript grammar, but given the way the dependency to the Javascript grammar is constructed, I think I first need to wait for this pull request to be merged before updating the Typescript pull request.

Checklist:

[X] All tests pass in CI.
[X] The script/parse-examples passes without issues.
[X] There are sufficient tests for the new fix/feature.
[X] Grammar rules have not been renamed unless absolutely necessary.
[X] The conflicts section hasn't grown too much.
[X] The parser size hasn't grown too much (check the value of STATE_COUNT in src/parser.c).

mjambon commented 1 year ago

This looks good to me. Sorry for the delay.

mjambon commented 1 year ago

There seems to be a setup issue for the Windows CI. See https://github.com/tree-sitter/tree-sitter-javascript/runs/7757868479?check_suite_focus=true @dcreager @patrickt @maxbrunsfeld

D:\a\tree-sitter-javascript\tree-sitter-javascript>if not defined npm_config_node_gyp (node "C:\hostedtoolcache\windows\node\14.20.0\x64\node_modules\npm\node_modules\npm-lifecycle\node-gyp-bin\\..\..\node_modules\node-gyp\bin\node-gyp.js" rebuild )  else (node "C:\hostedtoolcache\windows\node\14.20.0\x64\node_modules\npm\node_modules\node-gyp\bin\node-gyp.js" rebuild ) 
gyp ERR! find VS 
gyp ERR! find VS msvs_version not set from command line or npm config
gyp ERR! find VS VCINSTALLDIR not set, not running in VS Command Prompt
gyp ERR! find VS unknown version "undefined" found at "C:\Program Files\Microsoft Visual Studio\2022\Enterprise"
gyp ERR! find VS could not find a version of Visual Studio 2017 or newer to use
gyp ERR! find VS looking for Visual Studio 20[15](https://github.com/tree-sitter/tree-sitter-javascript/runs/7757868479?check_suite_focus=true#step:4:16)
gyp ERR! find VS - not found
gyp ERR! find VS not looking for VS2013 as it is only supported up to Node.js 8
gyp ERR! find VS 
gyp ERR! find VS **************************************************************
gyp ERR! find VS You need to install the latest version of Visual Studio
gyp ERR! find VS including the "Desktop development with C++" workload.
gyp ERR! find VS For more information consult the documentation at:
gyp ERR! find VS https://github.com/nodejs/node-gyp#on-windows
gyp ERR! find VS **************************************************************
gyp ERR! find VS 
gyp ERR! configure error 
gyp ERR! stack Error: Could not find any Visual Studio installation to use
...

aryx commented 1 year ago

I'm not super convinced by the member_expression change. You can't have any member_expression I think inside a JSX element; it can only be a set of identifiers. You can't have (<complex_expr).Bar, which is also a member_expression, so I'd rather keep things more precise.

aryx commented 1 year ago

Otherwise looks good to me.

guillaumebrunerie commented 1 year ago

Just to be clear, the member_expression change does not allow an arbitrary member_expression inside JSX, it still only allows nested identifiers as before. The only difference is that they will be aliased to member_expression so that highlighting rules for member_expressions apply there as well. I think it's reasonable for Foo.Bar to always be highlighted in the same way whether it's in JSX or in an expression.

aryx commented 1 year ago

Can you rebase on the latest to resolve the conflicts?

guillaumebrunerie commented 1 year ago

Ok, I believe I have now resolved the conflicts.

H4ckint0sh commented 1 year ago

will this be merged anytime soon?

SamStenner commented 11 months ago

Would love for this to be merged

amaanq commented 11 months ago

Can you rebase @guillaumebrunerie (last time :grin:)

guillaumebrunerie commented 11 months ago

@amaanq I have now rebased my branch, regenerated the files using the latest versions of tree-sitter and emcc, and squashed the commits.

amaanq commented 11 months ago

Great, thanks

tree-sitter / tree-sitter-javascript

Various fixes for JSX support #227