microsoft / rushstack

Monorepo for tools developed by the Rush Stack community
https://rushstack.io/
Other
5.93k stars 597 forks source link

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

Open 0618 opened 3 months ago

0618 commented 3 months ago

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 generatesAPI.md.api.md

Details

It might related to https://github.com/microsoft/rushstack/pull/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 commented 3 months ago

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 https://github.com/microsoft/rushstack/pull/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:

https://github.com/microsoft/rushstack/blob/0e14ced009489874137403403a896372b50dedef/apps/api-extractor/src/api/IConfigFile.ts#L91-L106

Related code changes: https://github.com/microsoft/rushstack/blob/0e14ced009489874137403403a896372b50dedef/apps/api-extractor/src/api/ExtractorConfig.ts#L934-L949

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.

sobolk commented 3 months ago

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 commented 3 months ago

This PR clarifies the documentation: https://github.com/microsoft/rushstack/pull/4846

octogonz commented 3 months ago

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 commented 3 months ago

@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.