tree-sitter-javascript icon indicating copy to clipboard operation
tree-sitter-javascript copied to clipboard

Various fixes for JSX support

Open guillaumebrunerie opened this issue 3 years ago • 8 comments

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).

guillaumebrunerie avatar Aug 01 '22 15:08 guillaumebrunerie

This looks good to me. Sorry for the delay.

mjambon avatar Aug 10 '22 01:08 mjambon

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
...

mjambon avatar Aug 10 '22 01:08 mjambon

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 avatar Aug 18 '22 09:08 aryx

Otherwise looks good to me.

aryx avatar Aug 18 '22 09:08 aryx

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.

guillaumebrunerie avatar Aug 18 '22 10:08 guillaumebrunerie

Can you rebase on the latest to resolve the conflicts?

aryx avatar Aug 18 '22 11:08 aryx

Ok, I believe I have now resolved the conflicts.

guillaumebrunerie avatar Aug 28 '22 15:08 guillaumebrunerie

will this be merged anytime soon?

H4ckint0sh avatar Sep 14 '22 11:09 H4ckint0sh

Would love for this to be merged

SamStenner avatar Jul 11 '23 08:07 SamStenner

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

amaanq avatar Jul 11 '23 10:07 amaanq

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

guillaumebrunerie avatar Jul 11 '23 12:07 guillaumebrunerie

Great, thanks

amaanq avatar Jul 11 '23 14:07 amaanq