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] [api-extractor] Incompatible release tags across package boundaries don't trigger ae-incompatible-release-tags with bundledPackages #4430

Open
Josmithr opened this issue Nov 27, 2023 · 4 comments
Labels
needs design The next step is for someone to propose the details of an approach for solving the problem repro confirmed The issue comments included repro instructions, and the maintainers reproduced the problem

Comments

@Josmithr
Copy link
Contributor

Josmithr commented Nov 27, 2023

Summary

Consider the following example where a mono-repo contains 2 packages: package-a and package-b, where package-b depends on package-a and specifies it in bundledPackages.

package-a

/**
 * @alpha
 */
export interface Foo {...}

package-b (depends on package-a)

import { Foo } from "package-a";

/**
 * @public
 */
export interface Bar extends Foo {...}

package-b's api-extractor config contains:

"bundledPackages": ["package-a"],

and

"ae-incompatible-release-tags": {
	"logLevel": "error",
	"addToApiReportFile": false
}

The expected outcome in this situation is that running api-extractor in package-b should trigger the ae-incompatible-release-tags error, but it doesn't.

Note that if Foo is re-exported by package-b, the error is triggered as expected. But since package-b specifies package-a in bundledPackages, the error should occur in either case.

Repro steps

I have created a small mono-repro that is configured with a fairly minimal repro of the issue: https://github.com/Josmithr/api-extractor-playground/tree/incompatible-release-tag-bug-repro

Standard questions

Please answer these questions to help us investigate your issue more quickly:

Question Answer
@microsoft/api-extractor version? 7.38.3
Operating system? Linux
API Extractor scenario? rollups (.d.ts)
Would you consider contributing a PR? No
TypeScript compiler version? 5.1.6
Node.js version (node -v)? 18.17.1
@Josmithr
Copy link
Contributor Author

I am likely missing some context here, but it seems like this check here should be extracted from the outer conditional - incompatible release tags and missing exports are orthogonal and should be verified independently.

@octogonz
Copy link
Collaborator

octogonz commented Dec 7, 2023

The expected outcome in this situation is that running api-extractor in package-b should trigger the ae-incompatible-release-tags error, but it doesn't.

I'm not sure about that.

bundledPackages is one of those simple-seeming features that leads to a pile of tricky edge cases. 😄

Here's two rather different use cases to consider:

  1. "My package imports someone else's NPM package, and but I'm bundling it "simpify" things." In this case, they want the other package's .d.ts files to get rolled up into your own .d.ts file. The other package maintainer may have their own designations about @alpha that are completely unrelated to the current package. It's also possible that they never heard of API Extractor and wrote @alpha in a comment with some totally different meaning, although isAedocSupportedFor() is supposed to detect that based on the absence of our tsdoc-metadata.json file.

  2. "I'm publishing one package, but for organizational purposes I want to split the code into multiple package folders that combined during the build." In this case, the same team manages all the source code, and it is possible for @alpha to have a consistent meaning across those projects. And I would agree for this case that it is expected for ae-incompatible-release-tags to have a consistent meaning.

As with your issue #4425, perhaps we need a setting to distinguish the intent? (Should it be a separate setting?)

@octogonz octogonz added needs more info We can't proceed because we need a better repro or an answer to a question needs design The next step is for someone to propose the details of an approach for solving the problem and removed needs more info We can't proceed because we need a better repro or an answer to a question labels Dec 7, 2023
@Josmithr
Copy link
Contributor Author

Josmithr commented Dec 7, 2023

Thanks for the response. Alongside your notes in #4425, I have a much better understanding of how these features were intended to compose, which didn't align with my understanding.

The distinction between these two cases is a fair one. I think I agree that a setting is probably sufficient to accomplish this.

But now that I better understand the intention of bundledPackages, I wonder if there are cases where both cases may want to be used simultaneously for orthogonal sets of dependencies? E.g. I want to bundle other packages in my mono-repo with validation of release tag compat, but I also want to bundle some external library, making no assumptions of its release semantics (i.e. treat everything from that as public).

A single, global option might be the easiest place to start, but I could also see an argument for allowing users to specify package-name/config pairs that specify the desired level of validation.

What do you think?

@octogonz octogonz added the repro confirmed The issue comments included repro instructions, and the maintainers reproduced the problem label Jan 23, 2024
@Josmithr
Copy link
Contributor Author

The other package maintainer may have their own designations about @Alpha that are completely unrelated to the current package. It's also possible that they never heard of API Extractor and wrote @Alpha in a comment with some totally different meaning, although isAedocSupportedFor() is supposed to detect that based on the absence of our tsdoc-metadata.json file.

@octogonz Given this, would unconditionally validating the tag compatibility when isAedocSupportedFor be a reasonable short-term mitigation for the issue here? I'm still very much in favor of a more intentional design for bundling, but I wonder if this is a reasonable short-term fix. My team is still maintaining a local pnpm patch for this, and we would love to stop maintaining that.

Josmithr added a commit to microsoft/FluidFramework that referenced this issue Apr 17, 2024
…e tag compatibility (#20696)

Mitigation for API-Extractor [issue
4430](microsoft/rushstack#4430).

The original mitigation we had in place was erroneously removed in
#19939.

Also fixes the handful of violations that have been introduced since
that PR.
Josmithr added a commit to Josmithr/FluidFramework that referenced this issue Apr 17, 2024
…e tag compatibility (microsoft#20696)

Mitigation for API-Extractor [issue
4430](microsoft/rushstack#4430).

The original mitigation we had in place was erroneously removed in

Also fixes the handful of violations that have been introduced since
that PR.
Josmithr added a commit to microsoft/FluidFramework that referenced this issue Apr 17, 2024
…e tag compatibility (#20696) (#20699)

Mitigation for API-Extractor [issue
4430](microsoft/rushstack#4430).

The original mitigation we had in place was erroneously removed in
#19939.

Also fixes the handful of violations that have been introduced since
that PR.

Cherry-picked from #20696
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs design The next step is for someone to propose the details of an approach for solving the problem repro confirmed The issue comments included repro instructions, and the maintainers reproduced the problem
Projects
Status: AE/AD
Development

No branches or pull requests

2 participants