-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Skip parsing JSDoc when not needed #52921
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
Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>
@typescript-bot perf test this |
Heya @jakebailey, I've started to run the perf test suite on this PR at 131a170. You can monitor the build here. |
This PR is still wrong, fixing but I have to run. |
@typescript-bot perf test this faster |
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 3af4435. You can monitor the build here. Update: The results are in! |
@typescript-bot test this |
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at 3af4435. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 3af4435. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 3af4435. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 3af4435. You can monitor the build here. |
Heya @jakebailey, I've started to run the extended test suite on this PR at 3af4435. 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 3af4435. You can monitor the build here. Update: The results are in! |
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 |
@jakebailey Here they are:Comparison Report - main..52921
System
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the user test suite comparing Something interesting changed - please have a look. Details
|
@jakebailey Here are the results of running the user test suite comparing Something interesting changed - please have a look. Detailswebpack
|
Um, hm. Those errors are interesting because they imply that I'm still doing something wrong here. |
f99893e
to
6930f47
Compare
@fisker @sosukesuzuki you might be interested in this one. I'm not sure if prettier or typescript-estree actually use the Actually, thinking about it, external users may not even need (It's kinda now or never, given the PR currently has a boolean which explicitly calls out semantic JSDoc tags.) |
In fact, one may not even need the tags at all even outside of TS files, e.g. formatting JS files... |
Actually to start, I'm just going to make this API internal as-is (because we want this in tsc now), and then send a follow-up change for comment to make this more configurable. |
@typescript-bot perf test this faster |
Heya @jakebailey, I've started to run the tsc-only perf test suite on this PR at b287a9f. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the bun perf test suite on this PR at b287a9f. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Follow-up is #55739. |
Yes. TS-ESTree does not use the JSDoc property at all - we do absolutely no touching of JSDoc. We do touch the comment contents so we can transform the AST node, but that is just via the source code text. I don't believe any part of our ecosystem touches JSDoc - as we've seen even the JSDoc plugin doesn't as it uses its own separate JSDoc parser. We'd see pretty decent wins across the ecosystem by being able to completely skip the JSDoc parsing! |
I think my main concern is that people will want to read out the But #55739 is probably a better place to keep talking about the bulk hammer of letting people disable it. |
Prettier uses the AST provied by |
Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Co-authored-by: Sheetal Nandi <shkamat@microsoft.com>
This is #52466 plus more changes, since I think we want to try and get this in for 5.0 if possible.
Needs a test, which we can achieve thanks to the compilerOptions use here.For #52959