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

docs: add api-extractor report #662

Merged
merged 14 commits into from
May 21, 2020

Conversation

markov00
Copy link
Member

@markov00 markov00 commented May 4, 2020

Summary

This PR activates the api-extractor report allowing a semi-automated check about possible breaking changes on the current published API contract.
The extractor outputs a Markdown report file called api/charts.api.md. The following is an excerpt of that file:

API Report File for "@elastic/charts"

Do not edit this file. It is a report generated by API Extractor.

import { $Values } from 'utility-types';
import React from 'react';
// @public
export type AccessorFn = UnaryAccessorFn;
// @public
export type AnnotationDomainType = $Values<typeof AnnotationDomainTypes>;
// @public
export const AnnotationDomainTypes: Readonly<{
    XDomain: "xDomain";
    YDomain: "yDomain";
}>;
// Warning: (ae-missing-release-tag) "AnnotationId" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public (undocumented)
export type AnnotationId = string;
// Warning: (ae-missing-release-tag) "AnnotationSpec" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public (undocumented)
export type AnnotationSpec = LineAnnotationSpec | RectAnnotationSpec;
// Warning: (ae-missing-release-tag) "AnnotationTooltipFormatter" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public (undocumented)
export type AnnotationTooltipFormatter = (details?: string) => JSX.Element | null;
// Warning: (ae-missing-release-tag) "AnnotationType" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public (undocumented)
export type AnnotationType = $Values<typeof AnnotationTypes>;

...
...

This file shows all the public/beta/alpha type/interface/function exposed as part of our public API (visible when importing elastic/charts into a project).

It includes a set of warnings related to missing tags or wrongly used tags, for example:

// Warning: (ae-missing-release-tag) "AnnotationSpec" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public (undocumented)
export type AnnotationSpec = LineAnnotationSpec | RectAnnotationSpec;

// src/chart_types/partition_chart/layout/types/config_types.ts:112:5 - (ae-forgotten-export) The symbol "TimeMs" needs to be exported by the entry point index.d.ts

The warnings in this example can be interpreted as:

  • AnnotationSpec type is undocumented (no TSDoc is provided)
  • The release tag (@alpha,@beta,@public) is missing
  • a type, in particular TimeMs, used by other exported types, is not exported by the main entry point.

This PR adds also a GH workflow that runs the yarn api:check commands as a GH check and warns the PR author about possible changes of the current API document.

Locally, you can run the same check running yarn api:check --local adding the --local will automatically update the api/charts.api.md file.

When this will be merged and during a PR review, it will be always required to take a careful look at the api/charts.api.md. If something breaks the existing contracts like a type declaration is changed, renamed, or removed then we should mark the PR as breaking change with a Screenshot 2020-05-06 at 16 36 52 label.

Future tasks:

  • cleanup existing API
  • automatically adds the breaking change label if the API contract is broken
  • generate a small breaking change changelog and add that to the PR description

@markov00 markov00 force-pushed the 2020_05_04-api-extractor branch from b40299d to b549438 Compare May 5, 2020 15:39
@codecov-io
Copy link

codecov-io commented May 5, 2020

Codecov Report

Merging #662 into master will increase coverage by 0.04%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #662      +/-   ##
==========================================
+ Coverage   72.94%   72.98%   +0.04%     
==========================================
  Files         266      266              
  Lines        8618     8632      +14     
  Branches     1695     1697       +2     
==========================================
+ Hits         6286     6300      +14     
  Misses       2293     2293              
  Partials       39       39              
Impacted Files Coverage Δ
...types/partition_chart/layout/types/config_types.ts 100.00% <ø> (ø)
...es/partition_chart/layout/types/viewmodel_types.ts 80.00% <ø> (ø)
...artition_chart/renderer/canvas/canvas_renderers.ts 6.20% <0.00%> (ø)
src/chart_types/xy_chart/utils/specs.ts 100.00% <ø> (ø)
src/components/chart.tsx 73.23% <ø> (ø)
src/scales/scale_band.ts 98.07% <ø> (ø)
src/scales/scale_continuous.ts 97.87% <ø> (ø)
src/specs/settings.tsx 90.90% <ø> (ø)
src/utils/accessor.ts 81.81% <ø> (ø)
src/utils/commons.ts 97.00% <ø> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4421748...20e7a7e. Read the comment docs.

@markov00 markov00 added :build Build tools / dependencies :docs Anything related to documentation, API, storybook labels May 6, 2020
@markov00 markov00 marked this pull request as ready for review May 6, 2020 14:40
Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

LGTM just a few comments.

src/chart_types/xy_chart/utils/specs.ts Show resolved Hide resolved
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License. */

import { ComponentType } from 'react';
import React from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this not work with selective imports?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the issue here is different, we should always import React on TSX files to avoid issues with the api-extractor that's need that all files using JSX should import React in a consistent way microsoft/rushstack#1765

@@ -1,7 +1,7 @@
{
"compilerOptions": {
"noUnusedLocals": true,
"removeComments": true
"removeComments": false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the extractor not work if this is set to true? If so I wonder if we should just have a separate tsconfig for the API documenter.

Copy link
Member Author

Choose a reason for hiding this comment

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

What I came up with is the following:
we can first generate the declaration files with all the comments, and then we recompile again only generating the js (no declarations) with comments.
This will increase the build type, but also avoid any third party library that can touch the generated code in an unpredicted way

Copy link
Member Author

Choose a reason for hiding this comment

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

here the commit: b4edfa3

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2020

Codecov Report

Merging #662 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #662   +/-   ##
=======================================
  Coverage   72.60%   72.60%           
=======================================
  Files         266      266           
  Lines        8678     8678           
  Branches     1710     1710           
=======================================
  Hits         6301     6301           
  Misses       2338     2338           
  Partials       39       39           
Impacted Files Coverage Δ
src/chart_types/xy_chart/utils/specs.ts 100.00% <ø> (ø)
src/components/chart.tsx 73.23% <ø> (ø)
src/scales/scale_band.ts 98.07% <ø> (ø)
src/scales/scale_continuous.ts 97.87% <ø> (ø)
src/specs/settings.tsx 90.90% <ø> (ø)
src/utils/accessor.ts 81.81% <ø> (ø)
src/utils/commons.ts 97.00% <ø> (ø)
src/chart_types/goal_chart/specs/index.ts 100.00% <100.00%> (ø)
...ypes/partition_chart/layout/viewmodel/viewmodel.ts 89.23% <100.00%> (ø)
src/chart_types/xy_chart/specs/rect_annotation.tsx 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5b4767...4f9a12e. Read the comment docs.

markov00 added 3 commits May 21, 2020 18:11
This commit adds comments to the generated declaration files but removes them from the compiled code. Unfortunately tsc doesn't do that by default, so we can only first generate the declaration with comments and then compile without
@markov00 markov00 merged commit 8856878 into elastic:master May 21, 2020
@markov00 markov00 deleted the 2020_05_04-api-extractor branch May 21, 2020 17:45
@markov00
Copy link
Member Author

🎉 This PR is included in version 19.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:build Build tools / dependencies :docs Anything related to documentation, API, storybook released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants