-
Notifications
You must be signed in to change notification settings - Fork 2.9k
docs(rfc): add api access modifiers style-guide RFC for v9 #23577
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
Changes from all commits
915b546
f138d72
e2f7505
dba9b80
d458514
121875e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,266 @@ | ||
| # RFC: Package public API accessors | ||
|
|
||
| --- | ||
|
|
||
| _@hotell_ | ||
|
|
||
| ## Summary | ||
|
|
||
| We want to control which parts of API surface of our packages are being exposed to public. | ||
|
|
||
| ## Background | ||
|
|
||
| During v9 pre-release phase we ran into various situation where we didn't want to propagate APIs as ready to go, thus we established `_unstable` identifiers naming suffix to mark those as unstable. To complement this approach we also introduced `*/unstable` deep imports for API's that are not STABLE. This works to some extent but fails short as it's a manual work that is not checked by automation. This led to scenarios that we unintentionally exposed INTERNAL apis to public which led to breaking changes which we want to avoid at all costs as we follow proper semver. | ||
|
|
||
| ## Problem statement | ||
|
|
||
| We expose all APIs of our packages as a PUBLIC from barrel files (`src/index.ts`) to consumers, which introduces problems as we can expose apis that were supposed to be used only internally within our repo or only be available in particular pre-release stage (ALPHA/BETA versions). | ||
|
|
||
| ## Detailed Design or Proposal | ||
|
|
||
| We want to provide style guide/common approach how to annotate package APIs with proper ACCESS modifiers which will be processed/verified by tooling automation. | ||
|
|
||
| Note that our approach will be applied only on **Type Declaration emit level** as we consider TypeScript surface as only source of truth for our package APIs. | ||
|
|
||
| > This means that if users use only vanilla JavaScript nothing will stop them from using those API's - as we don't support that kind of usage users take the risk that things may/will break for them unfortunately and also they will not get any support from our side. | ||
|
|
||
| ### Proposed access modifiers: | ||
|
|
||
| #### `@internal` | ||
|
|
||
| It indicates that an API item is meant only for usage by other NPM packages from the same repo. Third parties _should never use “internal” APIs._ | ||
|
|
||
| - tooling will trim the declaration from a public API surface. | ||
| - tooling will trim the declaration from `/unstable` API surface. | ||
|
|
||
| **Example:** | ||
|
|
||
| ```ts | ||
| // @filename package-a/src/index.ts | ||
|
|
||
| /** | ||
| * | ||
| * @internal | ||
| */ | ||
| export function doSomething(): void {} | ||
|
|
||
| export function hello(): void {} | ||
| ``` | ||
|
|
||
| ↓↓ transforms to ↓↓ | ||
|
|
||
| ```ts | ||
| // @filename package-a/dist/index.d.ts | ||
|
|
||
| export declare function hello(): void; | ||
| ``` | ||
|
|
||
| #### `@alpha` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not quite sure how useful the prerelease stage modifiers will be since it's harder for partners to be aware of these tags, not all IDEs will render tsdoc the same way. For the scenarios that the prerelease modifiers cover, IMO
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
good point, lemme provide a visual for the usage graph TD
A["@alpha,@beta"] --> U["@fluentui/pacakge-name/unstable"]
B["@public"] --> S["@fluentui/pacakge-name"]
C["@internal"] --> E((X))
these access modifiers will:
additional context: if needed we can allow Is that more clear now ? If yes I'll add the graphics to RFC. ty!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
As To check my understanding: we have following chain for packages
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this mean that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yes that's the idea (from TS api POV ) |
||
|
|
||
| It indicates that an API item is eventually intended to be public, but currently is in an early stage of development | ||
|
|
||
| - tooling will trim the declaration from a public API surface. | ||
| - can be exposed only from `/unstable` API surface | ||
| - tolling will NOT TRIM the declaration from `/unstable` API surface. | ||
|
|
||
| **Example:** | ||
|
|
||
| - unstable api | ||
|
|
||
| ```ts | ||
| // @filename package-a/src/unstable/index.ts | ||
|
|
||
| /** | ||
| * | ||
| * @alpha | ||
| */ | ||
| export function doSomething(): void {} | ||
|
|
||
| export function hello(): void {} | ||
| ``` | ||
|
|
||
| ↓↓ transforms to ↓↓ | ||
|
|
||
| ```ts | ||
| // @filename package-a/dist/unstable.d.ts | ||
| export function doSomething(): void; | ||
| export declare function hello(): void; | ||
| ``` | ||
|
|
||
| - stable api | ||
|
|
||
| ```ts | ||
| // @filename package-a/src/index.ts | ||
|
|
||
| // 🚨🚨🚨 NOTE THAT THIS IS PROBABLY A MISTAKE BUT TOOLING WILL COVER YOU | ||
| /** | ||
| * | ||
| * @alpha | ||
| */ | ||
| export function doSomething(): void {} | ||
| /** | ||
| * | ||
| * @public | ||
| */ | ||
| export function hello(): void {} | ||
| ``` | ||
|
|
||
| ↓↓ transforms to ↓↓ | ||
|
|
||
| ```ts | ||
| // @filename package-a/dist/index.d.ts | ||
| export declare function hello(): void; | ||
| ``` | ||
|
|
||
| #### `@beta` | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see any difference between in IMO it will reduce maintenance cost as with current proposal we should ensure that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
correct. I added more context to one of Lings comments
that's great idea ! we would need to get this implemented to api-extractor as it has limited support for access modifiers stripping / or eventually introduce custom tooling
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was looking more into custom annotations and api-extractor support and there might be some possibilities to do this. api-extractor documenter already supports
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| It indicates that an API item has been released as a preview or for experimental purposes. Third parties are encouraged to try it and provide feedback. However, a “beta” API should NOT be used in production, because it may be changed or removed in a future version. | ||
|
|
||
| - tooling will trim the declaration from a public API surface. | ||
| - can be exposed only from `/unstable` API surface | ||
| - tooling will NOT TRIM the declaration from `/unstable` API surface. | ||
|
|
||
| **Example:** | ||
|
|
||
| - unstable api | ||
|
|
||
| ```ts | ||
| // @filename package-a/src/unstable/index.ts | ||
|
|
||
| /** | ||
| * | ||
| * @beta | ||
| */ | ||
| export function doSomething(): void {} | ||
|
|
||
| export function hello(): void {} | ||
| ``` | ||
|
|
||
| ↓↓ transforms to ↓↓ | ||
|
|
||
| ```ts | ||
| // @filename package-a/dist/unstable.d.ts | ||
| export function doSomething(): void; | ||
| export declare function hello(): void; | ||
| ``` | ||
|
|
||
| - stable api | ||
|
|
||
| ```ts | ||
| // @filename package-a/src/index.ts | ||
|
|
||
| // 🚨🚨🚨 NOTE THAT THIS IS PROBABLY A MISTAKE BUT TOOLING WILL COVER YOU | ||
| /** | ||
| * | ||
| * @beta | ||
| */ | ||
| export function doSomething(): void {} | ||
| /** | ||
| * | ||
| * @public | ||
| */ | ||
| export function hello(): void {} | ||
| ``` | ||
|
|
||
| ↓↓ transforms to ↓↓ | ||
|
|
||
| ```ts | ||
| // @filename package-a/dist/index.d.ts | ||
| export declare function hello(): void; | ||
| ``` | ||
|
|
||
| #### `@public` | ||
|
|
||
| It indicates that an API item has been officially released, and is now part of the supported contract for a package. Semver versioning is used and enforced thus the API signature cannot be changed without a MAJOR version increment. | ||
|
|
||
| - tooling will NOT trim the declaration from a public API surface. | ||
| - CANNOT be exposed from `/unstable` API surface | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like the only reason to use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yeah, API without annotation is implicitly considered as public -> We can enforce it with tooling, so missing annotation will fail on CI/build
It might feel like unnecessary churn for some devs to add this annotation all the time, on the other hand:
personally I have no strong opinions on this, besides having consistency
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO we should enforce it via ESLint to be sure that all APIs are properly annotated.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation. That really helped. As you said, it is an extra bit of typing/work but I like the fact that everything would then always be explicit. We uncovered quite a few accidentally public APIs during bug bash so I think it's worth being strict about. |
||
|
|
||
| **Example:** | ||
|
|
||
| ```ts | ||
| // @filename package-a/src/index.ts | ||
|
|
||
| /** | ||
| * | ||
| * @internal | ||
| */ | ||
| export function doSomething(): void {} | ||
| /** | ||
| * | ||
| * @public | ||
| */ | ||
| export function hello(): void {} | ||
| ``` | ||
|
|
||
| ↓↓ transforms to ↓↓ | ||
|
|
||
| ```ts | ||
| // @filename package-a/dist/index.d.ts | ||
| export declare function hello(): void; | ||
| ``` | ||
|
|
||
| ### Summary | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please check out the summary with Fufure work paragraph - based on our discussion if it makes sense to you and if we can proceed. @layershifter @spmonahan @ling1726 cheers! |
||
|
|
||
| Access modifiers introduced in this document will create following package API patterns: | ||
|
|
||
| ```mermaid | ||
| graph TD | ||
| A["@alpha,@beta"] -- access from path --> U["@fluentui/package-name/unstable"] | ||
| B["@public"] -- access from path --> S["@fluentui/package-name"] | ||
| C["@internal"] -- access from path --> E((X)) | ||
| ``` | ||
|
|
||
| - enforce us to not expose public/beta/internal by accident as part of public API | ||
| - guard us from exposing public/beta shipped from package `/unstable` api to be shipped in consuming package as `stable` API | ||
|
|
||
| ### Future work: | ||
|
|
||
| To follow all release stage types we should support also `@rc` annotation for "release candidate". | ||
|
|
||
| Even better, to simplify differentiation between `stable` and `unstable` we would like to introduce `@unstable` annotation that would replace `@alpha`/`@beta`/`@rc` as from API access perspective there isn't any native JS tooling yet that would notify all consumers that they are using alpha/beta/rc APIs. For that reason we will go with `unstable` subpath and `@unstable` annotation for better tooling automation within our codebase. | ||
|
|
||
| ### Open questions: | ||
|
|
||
| - should we keep `_unstable` suffix for identifiers ? | ||
Hotell marked this conversation as resolved.
Show resolved
Hide resolved
Hotell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - yes | ||
| - should we prefix `@internal` apis with `_` ? (problematic with react hooks) | ||
| - not at this time. we might bring this up in the future with potential suffix which would work for react hooks as well. | ||
|
|
||
| ### Pros and Cons | ||
|
|
||
| <!-- Enumerate the pros and cons of the proposal. Make sure to think about and be clear on the cons or drawbacks of this propsoal. If there are multiple proposals include this for each. --> | ||
|
|
||
| **Pros:** | ||
|
|
||
| - explicit API exposure to users | ||
| - enforced API surface by tooling | ||
| - no unintended API leaks to users | ||
| - existing tooling that enables this behaviors to some extent (api-extractor) | ||
|
|
||
| **Cons:** | ||
|
|
||
| - worse local DX: need to `generate api` for all package dependencies within repo to be able to generate api report and catch api issues | ||
| - no existing tooling that "just works" for all use-cases | ||
| - api-extractor has various issues thus we need to change our build workflow a bit | ||
| - https://github.com/microsoft/rushstack/issues/3445 | ||
| - https://github.com/microsoft/rushstack/issues/3444 | ||
| - https://github.com/microsoft/rushstack/issues/3443 | ||
|
|
||
| ## Discarded Solutions | ||
|
|
||
| <!-- As you enumerate possible solutions, try to keep track of the discarded ones. This should include why we discarded the solution. --> | ||
|
|
||
| ## Open Issues | ||
|
|
||
| <!-- Optional section, but useful for first drafts. Use this section to track open issues on unanswered questions regarding the design or proposal. --> | ||
|
|
||
| - https://github.com/microsoft/fluentui/issues/23343 | ||
| - https://github.com/microsoft/fluentui/pull/23335 | ||
|
|
||
| ### Tolling issues: | ||
|
|
||
| - https://github.com/microsoft/rushstack/issues/3558 | ||
| - https://github.com/microsoft/rushstack/issues/3445 | ||
| - https://github.com/microsoft/rushstack/issues/3444 | ||
| - https://github.com/microsoft/rushstack/issues/3443 | ||



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.
We could have @public be the modifier and have the tooling make all things @internal by default. This would force us to be explicit in what is exposed. If we forget the attribute, the method or type can't get published.
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 actually agree with this 👍 how easy it is to implement might be a differnt story
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.
interesting idea indeed @GeoffCoxMSFT ! that would work only for stable API's though, which is not our use-case as we have various stages of API/packages that are being shipped to users.
haha YES