-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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(ivy): Update @publicApi to @codeGenApi on ivy instructions #29820
Conversation
e024bd7
to
929c3ba
Compare
- Removes `@publicApi` annotation from ivy instructions - Adds new `@codeGenApi` annotation to ivy instructions - Updates ts_api_guardian to support the new annotation properly
929c3ba
to
ba59453
Compare
This should keep these out of the AIO API doc. They don't look public, so I assume that is the intention? |
You can preview e024bd7 at https://pr29820-e024bd7.ngbuilds.io/. |
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.
The PR LGTM, except I don't understand:
- Where is
@codeApiGen
defined. Assuming it is not yet defined and will defined in the future, what does it do. - Why are some
Δ
-prefixed function/interfaces/etc. still kept as@publicApi
? (E.g. inject, defineInjectable/defineInjector, sanitizeHtml and co.) - What is the motivation fro the commit (other than that @IgorMinar requested this change 😛).
That info should be captured on the commit message.
So, I am lacking context (especially given that Δ
-prefixed exports are currently [ignored for API docs]) 😟
But the PR LGTM wrt what I suspect is the intention 😁
checkThrows( | ||
{'file.d.ts': input}, | ||
'file.d.ts(3,7): error: Required jsdoc tags - One of the tags: "@stable", "@foo", "@bar" - must exist on `param`.', | ||
{paramTags: {requireAtLeastOne: ['stable', 'foo', 'bar']}}); | ||
}); | ||
}); |
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.
It would be good to also have tests with multiple requireAtLeastOne
tags and:
a. one of them being matched
b. all of them being matched
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.
Please address other people's comments LGTM otherwise
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.
sounds good to me on the i18n part
😞 |
Addresses comments made by @gkalpak in angular#29820
Addresses comments made by @gkalpak in angular#29820
…ngular#29820) - Removes `@publicApi` annotation from ivy instructions - Adds new `@codeGenApi` annotation to ivy instructions - Updates ts_api_guardian to support the new annotation properly PR Close angular#29820
Addresses comments made by @gkalpak in angular#29820 PR Close angular#29844
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
@publicApi
annotation from ivy instructions@codeGenApi
annotation to ivy instructions