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

refactor(compiler-cli): Don't extract type implementations #56489

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JeanMeche
Copy link
Member

@JeanMeche JeanMeche commented Jun 18, 2024

This is useful for cases like class FormControl is defined as interface but the actual implementation uses a private type export const FormControl: ɵFormControlCtor.

fixes #56144

Demo: https://ng-dev-previews-fw--pr-angular-angular-56489-adev-prev-1fannvm9.web.app/api/forms/FormControl#usage-notes

@pullapprove pullapprove bot requested a review from alxhub June 18, 2024 11:54
@JeanMeche JeanMeche added area: compiler Issues related to `ngc`, Angular's template compiler area: adev Angular.dev documentation adev: preview labels Jun 18, 2024
@ngbot ngbot bot added this to the Backlog milestone Jun 18, 2024
Copy link

github-actions bot commented Jun 18, 2024

Deployed adev-preview for 6d930af to: https://ng-dev-previews-fw--pr-angular-angular-56489-adev-prev-7lmdybs8.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a test (e.g. in https://github.com/angular/angular/blob/0d78a92431178be73342aeddda4b62d9e49138ba/packages/compiler-cli/test/ngtsc/doc_extraction/doc_extraction_filtering_spec.ts#L20) and ideally also a comment in getDeclarationOfSymbol with background on why this flag exists.

@JeanMeche JeanMeche force-pushed the fix/compiler-formcontrol branch 4 times, most recently from d13a390 to 049e797 Compare June 18, 2024 21:52
getExportsOfModule(node: ts.Node): Map<string, Declaration> | null {
getExportsOfModule(
node: ts.Node,
skipPrivateValueDeclarationTypes: boolean = false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler typically avoids using defaulted args to make every caller consider the value instead of assuming the default. This case is a bit debatable given that the flag isn't typically desired, so from that perspective I can see how keeping the default makes sense, but it does make me wonder if this is more appropriate as a class field passed via the constructor.

getExportsOfModule(node: ts.Node): Map<string, Declaration> | null {
getExportsOfModule(
node: ts.Node,
skipPrivateValueDeclarationTypes: boolean = false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be reverted now

@@ -30,10 +30,16 @@ import {isNamedClassDeclaration} from './util';
* reflector.ts implements static reflection of declarations using the TypeScript `ts.TypeChecker`.
*/

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ought to be the docblock for the parameter itself, or the constructor alternatively, not the class

in order for the docs to process function entry, this commit refactor function extraction by keeping the implementation as a the default entry and adds all the overloads into a separate array of entries.
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the updates!

@JeanMeche JeanMeche added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jul 10, 2024
@JeanMeche JeanMeche removed the action: merge The PR is ready for merge by the caretaker label Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adev: preview area: adev Angular.dev documentation area: compiler Issues related to `ngc`, Angular's template compiler target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API documentation of FormControl is broken
2 participants