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 setup #44

Merged
merged 10 commits into from
Oct 15, 2019
Merged

Api extractor setup #44

merged 10 commits into from
Oct 15, 2019

Conversation

tkamireh
Copy link
Contributor

This PR addresses #24 and sets up the API-Extractor. Worth noting the following

  1. The generated markdown is not particularly helpful, we need to hook up api-documenter or otherwise find a way to document the public api of each of our packages
  2. API-Extractor will now point you to source files instead of the declaration files (this is much easier to make sense of and make fixes with).
  3. There are some small changes to the tsdocs over some of the files (mostly using / as an escape sequence)
  4. The pr Pipeline will now validate that change files have been included and api has been verified (will tell you to update if they have not been updated).

@kenotron
Copy link
Member

kenotron commented Oct 1, 2019

@ecraig12345 is a really good person to review this since she just touched ours in fabric

@@ -0,0 +1,3 @@
{
"extends": "../../api-extractor.json"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kenotron We should do this extends thing in OUFR's API extractor configs too

Copy link
Member

@ecraig12345 ecraig12345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, though I'm not sure I'm the most qualified to review in this repo. Left a few comments.

@@ -48,7 +48,7 @@ export type IMergeRecursionHandler = (options: IMergeOptions, ...objs: any[]) =>
* create a new object.
*
* @param options - options driving behavior of the merge. See IMergeOptions for a description. Some basic combos would
* be {} - no recursion, { depth: -1 } - recurse infinitely
* be \{\} - no recursion, \{ depth: -1 \} - recurse infinitely
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small thing, I'm guessing you could get rid of the need for escapes by wrapping the {} and { depth: -1 } in backticks (which also improves readability in intellisense)

Suggested change
* be \{\} - no recursion, \{ depth: -1 \} - recurse infinitely
* be `{}` - no recursion, `{ depth: -1 }` - recurse infinitely

* <ThemeLayer>{(theme: ITheme) => {
* return (<MyComponent style={someStyleFn(theme, 'stylename')}/>);
* }}</ThemeLayer>
* \<ThemeLayer\>\{(theme: ITheme) =\> \{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wrap this whole code sample in triple backticks, you shouldn't need all the escapes.
image


const _isProduction = process.argv.indexOf('--production') > -1;

// For API-Extractor to point you to source files rather than .d.ts files
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kenotron We should fix this declarationMap thing in OUFR too

state: IPressableState;
}

// Warning: (ae-forgotten-export) The symbol "IDivProps" needs to be exported by the entry point index.d.ts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a good idea to look through these warnings and either export the things that it points out are missing, or figure out how to prevent them from being referenced by exported stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, likely I'll keep the warnings in the api reports and add an issue to clean up our exports more generally. There's a good deal of cleanup that could be done regarding exports.

@tkamireh tkamireh merged commit a64cc0a into microsoft:master Oct 15, 2019
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

Successfully merging this pull request may close these issues.

None yet

3 participants