-
Notifications
You must be signed in to change notification settings - Fork 13k
preserve declarations if @internal is mentioned in unrelated comment #57960
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
base: main
Are you sure you want to change the base?
Conversation
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
Co-authored-by: Jake Bailey <[email protected]>
Merged main for you. As far as I'm concerned, this PR is correct; I tested But I know we're pretty iffy on whether or not I'm personally still for it, regardless, but I'll bring this one up in a design meeting just to double check. |
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 have no idea how it isn't breaking the tests, but this PR will need to modify scanner.ts
to update jsDocSeeOrLink
to also treat @internal
as a JSDoc comment that needs to be parsed in TS files and an update to the docs for JSDocParsingMode
to say that @internal
is also parsed. Along with testing in jsDocParsing.ts
.
Probably need to make ParseForTypeErrors
the default in the compiler tests... Really thought I did that already.
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.
Oops, meant to mark as needing changes.
Close/reopening to rerun CI and show the failure. |
Took the liberty to apply the described change. |
Just checking: @typescript-bot test it |
Hey @jakebailey, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: react
Package: porty
|
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.
Those new DT errors are interesting; seems like a bug in the PR somewhere.
@jakebailey Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
The DT errors should have been fixed. |
@typescript-bot test it |
@typescript-bot test it |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Perf is unfortunately still greatly affected and needs to be looked into. |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Perf problem is likely caused by the regex change.
|
Funnily I clicked that link and reran it, and it told me that the new one was 6% faster; I think this particular result is just noisy. Given the benchmark is consistent with so few samples, a profile may provide more insight (which I plan to test). |
LOL. After I executed it many times, I also found that the output result is somewhat random. |
Do the random perf results mean this is safe to merge? I'd guess not, unless it's a problem with the perf infrastructure. |
No, there's nothing wrong with the perf infra, there really is a slowdown here that needs to be profiled. |
@typescript-bot perf test this faster I can't seem to get the slowdown locally... |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Fixes #57352