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
RHCLOUD-23014: Use API Extractor on distributable packages #184
RHCLOUD-23014: Use API Extractor on distributable packages #184
Conversation
abc89db
to
81159e7
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
81159e7
to
56a05de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing these changes still in hac-dev and hac-core now.
/screenshots | ||
**/node_modules | ||
**/dist | ||
**/.DS_Store |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vojtechszocs please add back **/.DS_Store
. These files are unique to Mac OS and should be omitted from version control. See https://stackoverflow.com/a/30246861
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that OS specific files that aren't related to the project shouldn't have to be excluded in the first place, assuming that you configure the OS not to generate them within the repo. Having them OS generated and then git-ignored is not the best solution, in my opinion.
After some quick research, it seems that Mac OS doesn't support per-directory .DS_Store
generation toggle, which is unfortunate since these files are just useless clutter from any other OS perspective.
That said, I'll add the .DS_Store
ignore back.
set -euo pipefail | ||
|
||
yarn install | ||
yarn build-libs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vojtechszocs if we are planning to run this in the pipeline, do we need to run the build? Or can we add docs generation to the end of test.sh
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, this script is meant for manual execution.
It's not called by scripts that get executed as part of the CI workflow (test.sh
or test-prow-e2e.sh
).
The reason why we run yarn install
and yarn build-libs
here is to ensure that we have up-to-date JSON doc model files generated by the API Extractor, before we run API Documenter on these doc model files.
Or can we add docs generation to the end of
test.sh
?
Generated API docs are currently git-ignored, with a planned follow-up RHCLOUD-23203 where we'd collect them (both hand-written ones as well as generated ones) and publish them somewhere.
API doc generation, collection and publishing is closely related to publishing new version of lib package(s) to npmjs.
For now, I'd suggest to keep this as a separate script, having the above follow-up in mind.
generate-api-docs.sh
Outdated
|
||
mkdir -p docs/generated && rm -rf docs/generated/* | ||
|
||
shopt -s globstar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vojtechszocs this command is not available on Mac OS :(
Is there another option here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I'll look for an alternative.
What we're basically doing here is copying matching files into a flat structure at the given directory.
test.sh
Outdated
@@ -21,5 +21,11 @@ yarn lint | |||
# Run unit tests | |||
yarn test | |||
|
|||
# Ensure that Git repo is still clean | |||
if [[ -n "$(git status --porcelain)" ]]; then | |||
echo "Git repository is not clean, make sure that all changes are committed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vojtechszocs I'm assuming this is specifically for the generated docs? Can we scope down the check to just uncommitted docs in the docs folder? I know several folks run this script locally and this will be very annoying with uncommitted changes during development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking, if a git repo is clean before building the project, it should remain clean after the build as well. This was the intended purpose of the check above. Any artifacts generated during the build should be either committed or git-ignored.
Originally, I wanted to commit the generated API docs to the git repo, but then decided to git-ignore them for now, given the above mentioned follow-up that puts them to actual use.
I know several folks run this script locally and this will be very annoying with uncommitted changes during development.
Stuff which is user specific, like TODO files for example, can be git-ignored, e.g. I have this in my ~/.gitignore
**/vszocs-TODO*.txt
which allows me to put e.g. vszocs-TODO-items.txt
file in any git repo I work on.
That said, I guess you're right that it may cause troubles for some folks, so I'll remove it.
@@ -1,3 +1,9 @@ | |||
/coverage | |||
/docs/generated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vojtechszocs I see we have /reports
(checked in) and /docs/generated
(ignored). Just confirming that is the flow you expect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is intended.
reports
directory contains API reports (one Markdown file per lib package) which list all APIs exposed by the given package. Whenever the API surface of some package changes, API extractor will detect the "what's currently generated vs. what's committed in reports
directory" discrepancy and force the change to be propagated to reports
directory, making it part of the code review.
docs/generated
is meant to contain docs (including temporary artifacts such as JSON doc model files) that are generated from the project sources. Running generate-api-docs.sh
script produces the following file structure:
docs/generated/
├── api
│ ├── ... lots of .md files ...
│ └── index.md
├── lib-core.api.json
├── lib-extensions.api.json
├── lib-utils.api.json
└── lib-webpack.api.json
JSON files directly at docs/generated
are JSON doc model files. From these files, docs/generated/api
content is generated via API Documenter tool. docs/generated/api/index.md
is the index page of generated API docs.
See RHCLOUD-23203 which is a follow-up task related to the generated API docs.
@@ -60,7 +62,6 @@ | |||
"@typescript-eslint/parser": "^5.3.1", | |||
"babel-loader": "^8.2.5", | |||
"cypress": "^9.7.0", | |||
"downlevel-dts": "^0.10.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for sanity/clarity - we will need to add this back in a later PR for the 4.13 OCP console dynamic plugins epic: https://issues.redhat.com/browse/CONSOLE-3304
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also @vojtechszocs we may want to consider splitting up unrelated changes in the future. It will reduce the diff (already very large) and make it faster for approval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed and sorry, I have a bad habit of putting lots of changes together sometimes 🥲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param {import('eslint').Rule.RuleContext} context | ||
* @param {import('estree').ImportDeclaration} node | ||
*/ | ||
const checkImport = (context, node) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vojtechszocs I'm a bit concerned we are overriding the default eslint behavior here. Can you give a brief summary of why this override of the default eslint import
behavior is required? Wondering if it's possible for us to fix the root issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated in packages/eslint-plugin-internal/src/rules/all-bases.js
// This rule has inconsistent behavior and poor monorepo support, replaced by lib-restricted-external-imports
'import/no-extraneous-dependencies': 'off',
The rule import/no-extraneous-dependencies
comes from eslint-plugin-import
and is enabled by default since we use airbnb
present which auto-enables it, along with a bunch of other recommended rules.
The above mentioned "inconsistent behavior" refers to the fact that sometimes external module imports are not processed properly. Try updating packages/eslint-plugin-internal/src/rules/all-bases.js
like so:
// 'import/no-extraneous-dependencies': 'off',
'lib-restricted-external-imports': ['off', /* .. */],
and then update packages/lib-core/src/utils/url.ts
like so:
import * as _ from 'lodash'; // this was lodash-es before
and finally run this command from repo root directory:
yarn eslint packages/lib-core
Even though the closest parent package.json
for url.ts
doesn't have lodash
in either dependencies
or peerDependencies
, the check will pass, but it should not. Even when you remove the lodash
dev-dependency from monorepo root package.json
, the check will still pass.
We need a reliable ESLint rule to guard the package dependencies
& peerDependencies
vs. external module imports consistency that works well with our monorepo structure. Furthermore, we need to scope this rule only to lib packages. And furthermore, for some packages (lib-core
) we need to exclude @monorepo/common
imports since that code is bundled as part of lib-core
. And there's ultimately no way we can do all that with import/no-extraneous-dependencies
rule.
This is no different than writing a simple Rollup plugin to do whatever tweaks or checks to meet our specific needs.
writeJSONFile({ | ||
fileName: 'build-meta.json', | ||
fileName: 'build-metadata.json', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vojtechszocs can we move the filename to the top of the file in a constant so it's easier to find and change in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the string build-metadata.json
occurs only here and then in lib package manifests which are JSON files.
That said, I'll do the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chatted with @florkbr and since we're not planning to change this string in future, we can keep it here.
packages/lib-extensions/src/index.ts
Outdated
* | ||
* @remarks | ||
* This package provides extension types and corresponding type guard functions | ||
* used to interpret these extensions at runtime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT @vojtechszocs maybe instead of these
extensions we say SDK
extensions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, will do the change.
@@ -7,7 +7,7 @@ import { | |||
Label, | |||
DropdownPosition, | |||
} from '@patternfly/react-core'; | |||
import * as _ from 'lodash'; | |||
import * as _ from 'lodash-es'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vojtechszocs I see in some other locations we removed the -es
and here we are adding it. Can we come up with a consistent losdah import across all of our packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Packages that use lib-react-esm.json
TS config base should only import from lodash-es
lib-core
lib-extensions
lib-utils
Packages that use lib-node-cjs.json
TS config base should only import from lodash
lib-webpack
This relates to dependencies listed in the given package.json
file and to the dependency vs. external import consistency we're trying to guard with our custom lib-restricted-external-imports
ESLint rule.
In other words, packages whose JS output is ES modules should use lodash-es
, while packages whose JS output is CommonJS should use lodash
. The main reason why lodash-es
exists in the first place is because the CommonJS lodash
build is very hard to tree shake by module bundlers, so people naturally favor the ESM build where possible.
@vojtechszocs I actually ran into some issues testing this in hac-core as openshift/hac-core#108 has not been merged yet 🤦 I'll update that PR tomorrow if you're willing to review it. |
/screenshots | ||
**/node_modules | ||
**/dist | ||
**/.DS_Store |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that OS specific files that aren't related to the project shouldn't have to be excluded in the first place, assuming that you configure the OS not to generate them within the repo. Having them OS generated and then git-ignored is not the best solution, in my opinion.
After some quick research, it seems that Mac OS doesn't support per-directory .DS_Store
generation toggle, which is unfortunate since these files are just useless clutter from any other OS perspective.
That said, I'll add the .DS_Store
ignore back.
@@ -1,3 +1,9 @@ | |||
/coverage | |||
/docs/generated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is intended.
reports
directory contains API reports (one Markdown file per lib package) which list all APIs exposed by the given package. Whenever the API surface of some package changes, API extractor will detect the "what's currently generated vs. what's committed in reports
directory" discrepancy and force the change to be propagated to reports
directory, making it part of the code review.
docs/generated
is meant to contain docs (including temporary artifacts such as JSON doc model files) that are generated from the project sources. Running generate-api-docs.sh
script produces the following file structure:
docs/generated/
├── api
│ ├── ... lots of .md files ...
│ └── index.md
├── lib-core.api.json
├── lib-extensions.api.json
├── lib-utils.api.json
└── lib-webpack.api.json
JSON files directly at docs/generated
are JSON doc model files. From these files, docs/generated/api
content is generated via API Documenter tool. docs/generated/api/index.md
is the index page of generated API docs.
See RHCLOUD-23203 which is a follow-up task related to the generated API docs.
set -euo pipefail | ||
|
||
yarn install | ||
yarn build-libs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, this script is meant for manual execution.
It's not called by scripts that get executed as part of the CI workflow (test.sh
or test-prow-e2e.sh
).
The reason why we run yarn install
and yarn build-libs
here is to ensure that we have up-to-date JSON doc model files generated by the API Extractor, before we run API Documenter on these doc model files.
Or can we add docs generation to the end of
test.sh
?
Generated API docs are currently git-ignored, with a planned follow-up RHCLOUD-23203 where we'd collect them (both hand-written ones as well as generated ones) and publish them somewhere.
API doc generation, collection and publishing is closely related to publishing new version of lib package(s) to npmjs.
For now, I'd suggest to keep this as a separate script, having the above follow-up in mind.
generate-api-docs.sh
Outdated
|
||
mkdir -p docs/generated && rm -rf docs/generated/* | ||
|
||
shopt -s globstar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I'll look for an alternative.
What we're basically doing here is copying matching files into a flat structure at the given directory.
@@ -60,7 +62,6 @@ | |||
"@typescript-eslint/parser": "^5.3.1", | |||
"babel-loader": "^8.2.5", | |||
"cypress": "^9.7.0", | |||
"downlevel-dts": "^0.10.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed and sorry, I have a bad habit of putting lots of changes together sometimes 🥲
* @param {import('eslint').Rule.RuleContext} context | ||
* @param {import('estree').ImportDeclaration} node | ||
*/ | ||
const checkImport = (context, node) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated in packages/eslint-plugin-internal/src/rules/all-bases.js
// This rule has inconsistent behavior and poor monorepo support, replaced by lib-restricted-external-imports
'import/no-extraneous-dependencies': 'off',
The rule import/no-extraneous-dependencies
comes from eslint-plugin-import
and is enabled by default since we use airbnb
present which auto-enables it, along with a bunch of other recommended rules.
The above mentioned "inconsistent behavior" refers to the fact that sometimes external module imports are not processed properly. Try updating packages/eslint-plugin-internal/src/rules/all-bases.js
like so:
// 'import/no-extraneous-dependencies': 'off',
'lib-restricted-external-imports': ['off', /* .. */],
and then update packages/lib-core/src/utils/url.ts
like so:
import * as _ from 'lodash'; // this was lodash-es before
and finally run this command from repo root directory:
yarn eslint packages/lib-core
Even though the closest parent package.json
for url.ts
doesn't have lodash
in either dependencies
or peerDependencies
, the check will pass, but it should not. Even when you remove the lodash
dev-dependency from monorepo root package.json
, the check will still pass.
We need a reliable ESLint rule to guard the package dependencies
& peerDependencies
vs. external module imports consistency that works well with our monorepo structure. Furthermore, we need to scope this rule only to lib packages. And furthermore, for some packages (lib-core
) we need to exclude @monorepo/common
imports since that code is bundled as part of lib-core
. And there's ultimately no way we can do all that with import/no-extraneous-dependencies
rule.
This is no different than writing a simple Rollup plugin to do whatever tweaks or checks to meet our specific needs.
packages/lib-extensions/src/index.ts
Outdated
* | ||
* @remarks | ||
* This package provides extension types and corresponding type guard functions | ||
* used to interpret these extensions at runtime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, will do the change.
@@ -24,7 +23,7 @@ type AppInitSDKProps = { | |||
* Add this at app-level to make use of app's redux store and pass configurations prop needed to initialize the app, preferred to have it under Provider. | |||
* It checks for store instance if present or not. | |||
* If the store is there then the reference is persisted to be used in SDK else it creates a new store and passes it to the children with the provider | |||
* @component AppInitSDK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is most likely related to https://www.reddit.com/r/reactjs/comments/i9347c/jsdocs_for_react_components/
@@ -7,7 +7,7 @@ import { | |||
Label, | |||
DropdownPosition, | |||
} from '@patternfly/react-core'; | |||
import * as _ from 'lodash'; | |||
import * as _ from 'lodash-es'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Packages that use lib-react-esm.json
TS config base should only import from lodash-es
lib-core
lib-extensions
lib-utils
Packages that use lib-node-cjs.json
TS config base should only import from lodash
lib-webpack
This relates to dependencies listed in the given package.json
file and to the dependency vs. external import consistency we're trying to guard with our custom lib-restricted-external-imports
ESLint rule.
In other words, packages whose JS output is ES modules should use lodash-es
, while packages whose JS output is CommonJS should use lodash
. The main reason why lodash-es
exists in the first place is because the CommonJS lodash
build is very hard to tree shake by module bundlers, so people naturally favor the ESM build where possible.
test.sh
Outdated
@@ -21,5 +21,11 @@ yarn lint | |||
# Run unit tests | |||
yarn test | |||
|
|||
# Ensure that Git repo is still clean | |||
if [[ -n "$(git status --porcelain)" ]]; then | |||
echo "Git repository is not clean, make sure that all changes are committed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking, if a git repo is clean before building the project, it should remain clean after the build as well. This was the intended purpose of the check above. Any artifacts generated during the build should be either committed or git-ignored.
Originally, I wanted to commit the generated API docs to the git repo, but then decided to git-ignore them for now, given the above mentioned follow-up that puts them to actual use.
I know several folks run this script locally and this will be very annoying with uncommitted changes during development.
Stuff which is user specific, like TODO files for example, can be git-ignored, e.g. I have this in my ~/.gitignore
**/vszocs-TODO*.txt
which allows me to put e.g. vszocs-TODO-items.txt
file in any git repo I work on.
That said, I guess you're right that it may cause troubles for some folks, so I'll remove it.
|
||
/** | ||
* @param {import('eslint').Rule.RuleContext} context | ||
* @param {import('estree').ImportDeclaration} node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7899344
to
66e08aa
Compare
66e08aa
to
79ac6aa
Compare
@vojtechszocs: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
shopt -s globstar | ||
cp packages/lib-*/dist/api/lib-*.api.json docs/generated | ||
for dir in packages/lib-*; do | ||
cp $dir/dist/api/$(basename $dir).api.json docs/generated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
/lgtm |
Fixes RHCLOUD-23014
This PR adds API Extractor and API Documenter tools, with their workflow applied on all distributable
lib
packages.API Extractor workflow
Executed as part of the package
build
script, API Extractor does the following:index.d.ts
bundle, e.g.packages/lib-core/dist/index.d.ts
packages/lib-core/dist/api/lib-core.api.md
packages/lib-core/dist/api/lib-core.api.json
Note 📝
rollup-plugin-dts
is dropped in favor of thed.ts
bundling feature of API Extractor. This means that Rollup is only used for generating theindex.js
bundle.API reports overview
API Extractor compares the currently generated API report file with one currently committed at
reports
directory.If the files don't match, API Extractor fails the build and instructs the developer to copy the currently generated file to
reports
directory. This way, changes in API reports become part of the code review.Note 📝
tsdoc-metadata.json
is not generated since we don't need it, see here and here for details.API docs overview
This PR adds
generate-api-docs.sh
script that copies all the currently generated API doc model files todocs/generated
directory and then invokes API Documenter to generate Markdown API docs atdocs/generated/api
.Note 📝
docs/generated
directory is git-ignored which means that thedocs/generated/api
content isn't currently tracked in the git repo.As a follow-up, we should create a new monorepo package with all developer oriented docs (API docs, READMEs etc.) collected and processed into static web pages ready for deployment on services like Surge.sh.
Changes due to API Extractor package analysis
PluginLoader
andPluginStore
are marked asprivate
index.d.ts
bundleprivate
method testability, I generally agree with points described here - code should be tested in a way similar to how the client would use it, which is through the public interfaceae-forgotten-export
warnings@packageDocumentation
to index files of alllib
packagesOther changes and improvements
lib-restricted-external-imports
which enforces imported external modules to be declared independencies
orpeerDependencies
within the closest parentpackage.json
index.js
bundle doesn't include any external module codeimport/no-extraneous-dependencies
replaced bylib-restricted-external-imports
replaceCode
custom Rollup plugin for Lodash-specific code manipulationlib-restricted-external-imports
guards the import vs. manifest dependency consistencydownlevel-dts
dropped for now since we don't need it (yet)lib
packagesdist/build-meta.json
renamed todist/build-metadata.json