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] auto add .api.md #4842

Open
0618 opened this issue Jul 20, 2024 · 5 comments
Open

[api-extractor] auto add .api.md #4842

0618 opened this issue Jul 20, 2024 · 5 comments
Assignees
Labels
needs more info We can't proceed because we need a better repro or an answer to a question

Comments

@0618
Copy link

0618 commented Jul 20, 2024

After version 7.40.1, started to generate new report files with .api.md.

Summary

In my config, I have "reportFileName": "API.md",, so it used to generate API.md for me.

However, with the new version, it ignores the old API.md, but started generates a API.md.api.md instead.

Repro steps

  1. in api-extractor.json, set "reportFileName": "API.md",
  2. run api-extractor

Expected result:
Old version generates API.md

Actual result:
New version generates API.md.api.md

Details

It might related to #4621

The new minor version upgrade should not contain any breaking change.

Standard questions

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

Question Answer
@microsoft/api-extractor version? 7.40.1
Operating system?
API Extractor scenario?
Would you consider contributing a PR? No.
TypeScript compiler version? ~5.2.0
Node.js version (node -v)? v20.11.1
@octogonz
Copy link
Collaborator

The filename was always intended to be something like my-package.api.md, not API.md. Although it wasn't strictly enforced, the docs are pretty clear about that convention:

image

The change in behavior was likely introduced by @Josmithr's recent work in PR #4621 to support the new reportVariants setting, which expands this pattern to support names like my-package.public.api.md and my-package.beta.api.md:

/**
* The set of report variants to generate.
*
* @remarks
* Each variant corresponds to a minimal release level, denoted by release tag in the TSDoc comment for each API item.
* E.g., the `beta` report variant will include all API items tagged `@beta` or higher (i.e. `@beta` and `@public`).
*
* The resulting API report file names will be derived from the {@link IConfigApiReport.reportFileName}.
* E.g., `foo.beta.api.md`.
* The only exception to this is the `complete` variant.
* This variant name will not be contained in the corresponding file name.
* I.e., `foo.api.md`.
*
* @defaultValue `['complete']`
*/
reportVariants?: ApiReportVariant[];

Related code changes:

const suffixIndex: number = apiReportConfig.reportFileName.indexOf(reportFileNameSuffix);
if (suffixIndex < 0) {
// `.api.md` extension was not specified. Use provided file name base as is.
reportFileNameBase = apiReportConfig.reportFileName;
} else {
// The system previously asked users to specify their filenames in a form containing the `.api.md` extension.
// This guidance has changed, but to maintain backwards compatibility, we will temporarily support input
// that ends with the `.api.md` extension specially, by stripping it out.
// This should be removed in version 8, possibly replaced with an explicit error to help users
// migrate their configs.
reportFileNameBase = apiReportConfig.reportFileName.slice(0, suffixIndex);
}
} else {
// Default value
reportFileNameBase = '<unscopedPackageName>';
}

We could try to detect and support .md as an alternative suffix to .api.md, but I'm uncertain whether the extra complexity is worth it.

@0618 is there a reason why you don't just use the normal naming convention? In a monorepo, there can be hundreds of libraries, so it's fairly important for the package name to be included in the filename.

@octogonz octogonz added the needs more info We can't proceed because we need a better repro or an answer to a question label Jul 22, 2024
@sobolk
Copy link

sobolk commented Jul 22, 2024

is there a reason why you don't just use the normal naming convention? In a monorepo, there can be hundreds of libraries, so it's fairly important for the package name to be included in the filename.

@octogonz We're nesting each API.md within a directory that already has package name included. So from our point of view including package name in report name is redundant. We've built plenty of automation around assumption that we can have <repo_root>/packages/<package_name>/API.md files present. So your change is breaking us now.

The filename was always intended to be something like my-package.api.md, not API.md. Although it wasn't strictly enforced, the docs are pretty clear about that convention

You're saying that it was "not enforced convention" which sounds more like advisory than something enforced. We opted out and now new version that always appends extension breaks us.

@octogonz
Copy link
Collaborator

octogonz commented Jul 22, 2024

This PR clarifies the documentation: #4846

@octogonz
Copy link
Collaborator

We've built plenty of automation around assumption that we can have <repo_root>/packages/<package_name>/API.md files present.

If we allow API.md and then you enable the reportVariants feature, how would the variants get named? public.md? api.public.md? public.api.md? All these options are plausible, but I'm not seeing an intuitive way to explain how api.md would transform into one of those patterns. (Whereas appending a suffix is straightforward and intuitive.)

Of course API Extractor could have provided separate settings for each variant report filename, but @Josmithr and I both thought that design would be awkward to configure. Also, standardizing the file extensions makes the reports easier to distinguish.

You're saying that it was "not enforced convention" which sounds more like advisory than something enforced. We opted out and now new version that always appends extension breaks us.

The docs say:

"The file extension should be .api.md, and the string should not contain a path separator such as \ or /."

There's even a diagram: 😄

image

Therefore, it's kind of a weak case that we have an obligation to allow other file extensions, just because it didn't fail with an error.

From a software design perspective, flexibility is a price, not a benefit. That price is worth paying when it enables us to solve new kinds of problems. But flexibility always makes it harder to support other requirements such as reportVariants. Simple tools can ignore this, because they have minimal requirements and don't care too much about breaking people, whereas an "enterprise" tool like API Extractor has to carefully weigh these concerns.

@sobolk
Copy link

sobolk commented Jul 24, 2024

@octogonz

The docs say:

"The file extension should be .api.md, and the string should not contain a path separator such as \ or /."

Therefore, it's kind of a weak case that we have an obligation to allow other file extensions, just because it didn't fail with an error.

Per https://www.ietf.org/rfc/rfc2119.txt should indicates recommendation, not a requirement. We tried API.md, it worked so we assumed it's ok.
I would say it could turn into long legal battle to figure out if it's user error (us ignoring/misunderstanding documentation) or provider mistake that broke runtime behavior. A strict runtime validation of inputs in addition to documentation would help avoiding this grey area if it was implemented.

From a software design perspective, flexibility is a price, not a benefit. That price is worth paying when it enables us to solve new kinds of problems. But flexibility always makes it harder to support other requirements such as reportVariants. Simple tools can ignore this, because they have minimal requirements and don't care too much about breaking people, whereas an "enterprise" tool like API Extractor has to carefully weigh these concerns.

I understand the value added in the flows you described. But, we're not using them at all, our interaction with extractor ends at report generation where we desire constant name of the report in our current setup. Therefore all the benefits you're describing don't matter much for our usage pattern.
From our point of view the extractor was working fine yesterday and it stopped working for our use case today without corresponding major version increment on your side. The fix on our side appears to be as simple as coming up with new name for this. But it's definitely unwanted work and source of frustration on our side.

dpilch added a commit to aws-amplify/amplify-category-api that referenced this issue Sep 5, 2024
dpilch added a commit to aws-amplify/amplify-category-api that referenced this issue Sep 6, 2024
dpilch added a commit to aws-amplify/amplify-category-api that referenced this issue Sep 6, 2024
dpilch added a commit to aws-amplify/amplify-category-api that referenced this issue Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more info We can't proceed because we need a better repro or an answer to a question
Projects
Status: AE/AD
Development

No branches or pull requests

3 participants