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

Update TypeScript lexer to allow nested generics #1002

Merged
merged 2 commits into from
Sep 21, 2024

Conversation

fredrare
Copy link
Contributor

#425 shows that generics are not correctly identified in general, but they are rather being treated as JSX elements. I proposed a simple solution in the comments by adding a space between < and the word next to it, but I believe most people will either not find the solution or some of them will find it rather unappealing.

For this reason, I made the JSX rules recursive and added a "," Punctuation token inside so that there can be a number of generics used, as well as allowing nested generics. While I am not really fond of this hack, given that generics are already treated as JSX elements, I think this is a fair and easy enough solution for most cases.

Before

Screenshot 2024-09-20 at 9 28 05 PM

With spacing solution

Screenshot 2024-09-20 at 9 30 13 PM

With recursive JSX and "," Punctuation token

Screenshot 2024-09-20 at 9 55 11 PM

@alecthomas
Copy link
Owner

Mind adding some JSX samples to the test? We should make sure that still works.

@fredrare
Copy link
Contributor Author

Sure!
I have just included a JSX example with usual attributes and syntax to the test (my bad for not including them before). It should work just fine with any old JSX elements. The only thing is that JSX now also allows for recursive elements, but the highlighter should render exactly the same output for any old valid JSX.

Screenshot 2024-09-21 at 5 58 42 AM

@alecthomas
Copy link
Owner

Awesome, thanks!

@alecthomas alecthomas merged commit a56e228 into alecthomas:master Sep 21, 2024
2 checks passed
@alecthomas
Copy link
Owner

Thanks for fixing that, appreciate it.

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.

2 participants