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] Design change: by default config paths should be relative to api-extractor.json folder #1216

Closed
octogonz opened this issue Apr 9, 2019 · 6 comments

Comments

@octogonz
Copy link
Collaborator

octogonz commented Apr 9, 2019

Issue #1017 fixed some weirdness with the resolution of relative paths in api-extractor.json and added documentation explaining how each field is resolved.

However after dogfooding this release, @iclanton felt strongly that all settings should use the same rules for resolution; otherwise when reading the file it is difficult to remember how each setting is resolved. The intuitive behavior would be for them to be relative to the folder of the file being processed (i.e. api-extractor.json or one of its extends references). But since most of the settings are specifying output file paths, it's more natural to specify them relative to the rootFolder. So he suggested to introduce a <rootFolder> token to make this explicit (which is similar to <rootDir> used by Jest).

@octogonz
Copy link
Collaborator Author

octogonz commented Apr 9, 2019

Also, I realized that API Extractor's rootFolder can now be sometimes different from the TypeScript compiler's rootDir option (which the compiler usually infers from other paths). Today rootFolder is not used any more to configure the compiler; basically it is only used when interpreting the config files.

The rootFolder can also be different from the WorkingPackage.packageFolder.

Thus, to avoid confusion I'm going to rename this setting to projectFolder.

@octogonz
Copy link
Collaborator Author

octogonz commented Apr 9, 2019

I created a PR #1217 that initially shows how api-extractor-template.json would look with this proposed change.

@iclanton what do you think? The resolution rules are definitely more obvious, but the amount of explanation required in the comments makes it seem overengineered. It would be nice to find a way to simplify it.

(Note that tsconfig.json avoids this problem by presumptuously assuming its file will go in the root folder of the project, but I wouldn't want that requirement for api-extractor.json.)

@octogonz
Copy link
Collaborator Author

octogonz commented Apr 9, 2019

It would be nice to find a way to simplify it.

I suppose this design is unavoidable unless we give up one of the two requirements: Either (1) we'd have to resolve "extends" differently from tsc and tslint, or (2) we'd have to use different resolution rules for different settings.

@skaapgif in case you had any thoughts

@rudolf
Copy link
Contributor

rudolf commented Apr 9, 2019

It is not immediately obvious to me why you would inherit from another api-extractor.json config file apart from perhaps sharing your messages configuration. So I'm struggling to imagine how you would extend a config file with paths.

Having said that, I agree that paths resolved relative to the location of the config file is the most obvious/intuitive for me. If there was no other documentation or comments this would be my assumption. The documentation is a bit more verbose, but it feels like the configuration is much more explicit which reduces uncertainty, so I like the <projectFolder> tag as an explicit way to prevent paths like ../../../output

@octogonz
Copy link
Collaborator Author

octogonz commented Apr 9, 2019

It is not immediately obvious to me why you would inherit from another api-extractor.json config file apart from perhaps sharing your messages configuration. So I'm struggling to imagine how you would extend a config file with paths.

Agreed. I think @iclanton's concern more about establishing the general convention that we will apply to any other fields that may be introduced in the future (as well for various other config files developed in this repo). For example Rush Stack toolchain relies heavily on extends inheritance and encountered some frustrations with config files that didn't perform resolution sensibly.

I went ahead and implemented the full change in the PR. It required partially resolving the extended config files as they are loaded, which muddies the separation between ExtractorConfig.loadFile() and ExtractorConfig.prepare(). This separation was supposed to allow options to be adjusted after they are loaded from the config file, which is still possible, although if I have second thoughts about the design I might mark that API as @beta before releasing.

@octogonz
Copy link
Collaborator Author

This was published with API Extractor beta release 7.0.41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants