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

[api-extractor] Allow separate release tags for merged declarations #972

Open
nvh opened this issue Nov 30, 2018 · 6 comments
Open

[api-extractor] Allow separate release tags for merged declarations #972

nvh opened this issue Nov 30, 2018 · 6 comments
Labels
enhancement The issue is asking for a new feature or design change needs design The next step is for someone to propose the details of an approach for solving the problem

Comments

@nvh
Copy link

nvh commented Nov 30, 2018

Reproduction

Export a merged type with two different release tags:

/**
 * @public
 */
interface Size {
    width: number
    height: number
}

/**
 * @beta
 */
function Size(width: number, height: number): Size {
    return { width, height }
}

Expected

The function definition only shows up in beta rollup file

Actual

The function definition shows up in the public rollup file too, including the @beta tag

@nvh
Copy link
Author

nvh commented Nov 30, 2018

@nvh nvh changed the title [api-extractor] separate releases tags for merge types are not supported [api-extractor] separate releases tags for merged types are not supported Dec 1, 2018
@octogonz octogonz added enhancement The issue is asking for a new feature or design change help wanted If you're looking to contribute, this issue is a good place to start! effort: easy Probably a quick fix. Want to contribute? :-) labels Feb 11, 2019
@octogonz
Copy link
Collaborator

We should do this. A while ago I confirmed with the compiler people that the semantics are probably sound for .d.ts trimming of individual declarations. The implementation should be pretty easy as of AE7.

@octogonz octogonz added priority The maintainers consider it to be an important issue. We should try to fix it soon. and removed help wanted If you're looking to contribute, this issue is a good place to start! labels Mar 10, 2019
@octogonz octogonz changed the title [api-extractor] separate releases tags for merged types are not supported [api-extractor] Allow separate release tags for merged declarations Mar 10, 2019
@octogonz
Copy link
Collaborator

octogonz commented Mar 17, 2019

I started looking at this again today. Turns out it introduces some complexities for API Extractor's warning regarding "incompatible release tags".

The warning helps by catching situations like this:

/** @beta */
interface I1 {
  x: number;
}

// Incompatible release tag: the function f1() is marked as public, but its signature
// relies on the interface "I1" which is marked as beta.  If the .d.ts rollup were to
// trim "I1", then callers of f1() won't be able to make the "i" parameter.
/** @public */
export function f1(i: I1): void {  // <-- not okay
}

// But it's okay for the signature to reference a release tag that has greater visibility:
/** @alpha*/
export function g1(i: I1): void { // <-- okay because beta >= alpha
}

When we allow different release tags for merged declarations, the analysis can get a little tricky:

/** @beta */
export namespace N2 {

} 

/** @public */
export interface N2 {
  x: number;
}

// Even though N2:namespace is beta, it's okay here because
// this expression is only referring to N2:interface
/** @public */
export function f2(n: N2): void {
}

And we get into trouble if we combine certain things:

/** @public */
export interface I3 {
  x: number;
}

// The two interfaces become the same semantic object, so it doesn't make sense
// for them to have different release tags.  API Extractor must require both
// interface declarations to have the same release tag.
/** @beta*/  // <-- not okay
export interface I3 {
  y: number;
}

Since a TypeScript class can be used as an interface, the same problem affects classes that get merged with interfaces:

/** @public */
export class C4 {
  x: number;
}
/** @beta*/  // <-- not okay
export interface C4 {
  y: number;
}

// If we trim C4:interface, then a caller cannot make a valid "c"
// (as an interface, not a class instance)
/** @public */
export function f4(c: C4): void {
}

Here's another pathological case:

/** @beta  */
enum E5 {
  X = 123
}

/** @public */
namespace E5 {
  export type Y = "a" | "b";
}

/** @public */
function f5(e: E5.X | E5.Y): void {  // <-- not okay

}

// This usage is possibly okay, but a nontrivial analysis is required
// because we don't know which "E5" we're referring to until we
// resolve "Y"
/** @public */
function g5(e: E5.Y): void { 
}

I feel like we can still implement this feature; it just requires a little more design thought. Rather than getting deep into type algebra, maybe we could identify a small set of cases that are obviously safe, and then slowly expand that set over time.

Either way, this work item seems harder than I originally estimated, so we'll have to cut it from the initial release of AE7.

@octogonz octogonz added needs design The next step is for someone to propose the details of an approach for solving the problem and removed priority The maintainers consider it to be an important issue. We should try to fix it soon. effort: easy Probably a quick fix. Want to contribute? :-) labels Mar 17, 2019
@octogonz
Copy link
Collaborator

@johnguy0 asked today about support for function overloads as a special case. For example:

/** @beta */
export function add(x: string, y: string): string;

/** @public */
export function add(x: number, y: number): number;

// implementation
export function add(x: string|number, y: string|number): string|number {
}

This seems relatively safe.

@octogonz
Copy link
Collaborator

octogonz commented Oct 6, 2019

Support for overloaded functions was published with API Extractor 7.5.0.

I'm still open to supporting this for other types of declarations, but (as noted above) someone needs to propose some constraints for what is safe to allow.

@AJIXuMuK
Copy link

AJIXuMuK commented Nov 30, 2023

@octogonz - another thing I have in mind is union types.
Something like

/**
 * @public
 */
export interface IPublicInterface {}

/**
 * @beta
 */
export interface IBetaInterface {}

/**
 * @internal
 */
export interface IInternalInterface {}

/**
 * This one one become
 *  - export type PublicType = IPublicInterface; for public release
 *  - export type PublicType = IPublicInterface | IBetaInterface; for beta release
 *  - export type PublicType = IPublicInterface | IBetaInterface | IInternalInterface; for internal
 *
 * @public
 */
export type PublicType = IPublicInterface | IBetaInterface | IInternalInterface;

@github-project-automation github-project-automation bot moved this to Needs triage in Bug Triage Feb 26, 2024
@iclanton iclanton moved this from Needs triage to AE/AD in Bug Triage Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue is asking for a new feature or design change needs design The next step is for someone to propose the details of an approach for solving the problem
Projects
Status: AE/AD
Development

No branches or pull requests

3 participants