-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Don't parse out a name for JSX fragments #52818
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
Conversation
@typescript-bot pack this |
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 5c31b9a. You can monitor the build here. |
Might as well use this to test the new tsserver tests. @typescript-bot user test tsserver |
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at 5c31b9a. You can monitor the build here. |
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at 5c31b9a. You can monitor the build here. |
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
@typescript-bot user test tsserver |
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at 5c31b9a. You can monitor the build here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's fine - would be nice if our recovery here didn't conflict with our rehydration strategy.
@jakebailey Here are the results of running the user test suite comparing Something interesting changed - please have a look. Details[acorn]
That is a filtered view of the text. To see the raw error text, go to RepoResults1/acorn.rawError.txt in the artifact folder
|
@jakebailey Here are some more interesting changes from running the user test suite Details[lodash]
That is a filtered view of the text. To see the raw error text, go to RepoResults3/lodash.rawError.txt in the artifact folder
|
@jakebailey Here are some more interesting changes from running the user test suite Detailswebpack
That is a filtered view of the text. To see the raw error text, go to RepoResults4/webpack.rawError.txt in the artifact folder
|
Okay, apparently I did not fix the server stuff . |
@typescript-bot user test tsserver |
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at 5c31b9a. You can monitor the build here. Update: The results are in! |
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
@typescript-bot test tsserver top100 |
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at 5c31b9a. You can monitor the build here. Update: The results are in! |
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
Just to clarify, you're meaning that it's unfortunate that we have to modify the parser to deal with this assert during |
Yeah, the rehydration strategy is at odds with some basic recovery tactics. It means that in a lot of cases, if you want to recover gracefully, you have to store error-recovery nodes in the tree. But if you store erroneous nodes in the tree, you probably need to make them accessible via But you also have to make sure that you deal with them correctly in the formatter and visitor - or else you'll run into issues. So to fix that, you also have to make decisions about how our factory functions handle these nodes - that they explicitly drop them or unset them (#52803 (comment)). This work all feels bad, so it's easier to just parse "worse". |
The other thing I was trying was to instead parse |
Fixes #44154
I don't know why I didn't think of doing this sooner; if we don't parse the name and then throw it away, we won't end up with the trivia problem. No weirdness happens at all, and we don't need to screw with the tree to try and recover.