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

RHCLOUD-23014: Use API Extractor on distributable packages #184

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .dockerignore
@@ -1,4 +1,5 @@
/coverage
/docs/generated
/screenshots
**/node_modules
**/dist
12 changes: 6 additions & 6 deletions .gitignore
@@ -1,3 +1,9 @@
/coverage
/docs/generated
Copy link
Contributor

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?

Copy link
Contributor Author

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.

/screenshots
**/node_modules
**/dist

# https://next.yarnpkg.com/getting-started/qa#which-files-should-be-gitignored
.pnp.*
.yarn/*
Expand All @@ -6,9 +12,3 @@
!.yarn/releases
!.yarn/sdks
!.yarn/versions

/coverage
/screenshots
**/node_modules
**/dist
**/.DS_Store
Copy link
Contributor

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

Copy link
Contributor Author

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.

7 changes: 7 additions & 0 deletions .vscode/settings.json
Expand Up @@ -14,6 +14,13 @@
"**/node_modules": true,
"**/dist": true
},
"files.associations": {
"**/api-extractor.json": "jsonc"
},

"eslint.options": {
"rulePaths": ["./packages/eslint-plugin-internal/src/runtime-rules"]
},

// https://code.visualstudio.com/docs/languages/identifiers
"[javascript]": {
Expand Down
45 changes: 45 additions & 0 deletions api-extractor.json
@@ -0,0 +1,45 @@
{
// Matches the end_of_line setting in .editorconfig
"newlineKind": "lf",

"dtsRollup": {
"enabled": true,
"untrimmedFilePath": "<projectFolder>/dist/index.d.ts"
},

"apiReport": {
"enabled": true,
"reportFolder": "reports",
"reportTempFolder": "<projectFolder>/dist/api"
},

"docModel": {
"enabled": true
},

// Avoid generating tsdoc-metadata.json
// https://github.com/microsoft/rushstack/pull/1628#issuecomment-553665782
"tsdocMetadata": {
"enabled": false
},

"messages": {
"extractorMessageReporting": {
// Avoid having to explicitly mark every exported API member with @public tag
"ae-missing-release-tag": {
"logLevel": "none"
},
// Avoid issues with TSDoc parser unable to process @link references
"ae-unresolved-link": {
"logLevel": "none"
}
},

"tsdocMessageReporting": {
// https://github.com/Microsoft/tsdoc/issues/19
"tsdoc-param-tag-with-invalid-name": {
"logLevel": "none"
}
}
}
}
12 changes: 12 additions & 0 deletions generate-api-docs.sh
@@ -0,0 +1,12 @@
#!/bin/bash
set -euo pipefail

yarn install
yarn build-libs
Copy link
Contributor

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?

Copy link
Contributor Author

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.


mkdir -p docs/generated && rm -rf docs/generated/*

shopt -s globstar
Copy link
Contributor

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?

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

cp packages/lib-*/dist/api/lib-*.api.json docs/generated

yarn api-documenter markdown -i docs/generated -o docs/generated/api
14 changes: 8 additions & 6 deletions package.json
Expand Up @@ -15,15 +15,17 @@
"test-e2e": "yarn workspace @monorepo/e2e-sample-app run test-e2e",
"test-e2e-open": "yarn workspace @monorepo/e2e-sample-app run test-e2e-open",
"test-e2e-ci": "start-server-and-test 'yarn run-samples' '9000|9001' 'yarn test-e2e'",
"eslint": "eslint --max-warnings 0 --ext .js,.jsx,.ts,.tsx",
"eslint-print-config": "eslint --print-config",
"eslint": "eslint --max-warnings 0 --ext .js,.jsx,.ts,.tsx --rulesdir ./packages/eslint-plugin-internal/src/runtime-rules",
"eslint-print-config": "yarn eslint --print-config",
"jest": "TZ=UTC jest --passWithNoTests",
"storybook": "start-storybook -p 6006",
"build-storybook": "build-storybook"
"api-extractor": "api-extractor run --typescript-compiler-folder ./node_modules/typescript/lib",
"storybook": "start-storybook -p 6006"
},
"devDependencies": {
"@babel/core": "^7.18.6",
"@mdx-js/react": "^1.6.22",
"@microsoft/api-documenter": "^7.19.24",
"@microsoft/api-extractor": "^7.33.6",
"@patternfly/react-core": "^4.202.16",
"@patternfly/react-table": "^4.71.16",
"@patternfly/react-virtualized-extension": "^4.53.2",
Expand Down Expand Up @@ -60,7 +62,6 @@
"@typescript-eslint/parser": "^5.3.1",
"babel-loader": "^8.2.5",
"cypress": "^9.7.0",
"downlevel-dts": "^0.10.0",
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

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 🥲

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bart

"eslint": "^7.32.0",
"eslint-config-airbnb": "^19.0.0",
"eslint-config-airbnb-base": "^15.0.0",
Expand All @@ -74,11 +75,13 @@
"eslint-plugin-promise": "^5.1.1",
"eslint-plugin-react": "^7.27.0",
"eslint-plugin-react-hooks": "^4.3.0",
"find-up": "^5.0.0",
"identity-obj-proxy": "^3.0.0",
"jest": "^27.5.1",
"jest-axe": "^6.0.0",
"lodash": "^4.17.21",
"lodash-es": "^4.17.21",
"minimatch": "^3.1.2",
"prettier": "^2.4.1",
"react": "^17.0.2",
"react-dom": "^17.0.2",
Expand All @@ -91,7 +94,6 @@
"redux-thunk": "^2.4.1",
"rollup": "^2.61.1",
"rollup-plugin-analyzer": "^4.0.0",
"rollup-plugin-dts": "^4.0.1",
"rollup-plugin-import-css": "^3.0.3",
"start-server-and-test": "^1.14.0",
"storybook-addon-react-router-v6": "^0.1.14",
Expand Down
64 changes: 5 additions & 59 deletions packages/common/rollup-configs.js
Expand Up @@ -3,9 +3,7 @@ import commonjs from '@rollup/plugin-commonjs';
import nodeResolve from '@rollup/plugin-node-resolve';
import typescript from '@rollup/plugin-typescript';
import analyzer from 'rollup-plugin-analyzer';
import dts from 'rollup-plugin-dts';
import css from 'rollup-plugin-import-css';
import { replaceCode } from './rollup-plugins/replaceCode';
import { writeJSONFile } from './rollup-plugins/writeJSONFile';

// https://yarnpkg.com/advanced/lifecycle-scripts#environment-variables
Expand Down Expand Up @@ -52,36 +50,8 @@ const getBanner = ({ repository }, buildMetadata) => {
/**
* @param {import('type-fest').PackageJson} pkg
*/
const getExternalModules = ({ dependencies, peerDependencies }) => {
const modules = new Set([
...Object.keys(dependencies ?? {}),
...Object.keys(peerDependencies ?? {}),
]);

modules.add('lodash-es');
modules.delete('lodash');

return Array.from(modules);
};

/**
* @param {import('type-fest').PackageJson} pkg
*/
const getExternalModuleRegExps = (pkg) => {
const externalModules = getExternalModules(pkg);
return externalModules.map((module) => new RegExp(`^${module}(\\/.+)*$`));
};

/**
* @param {string} fileName
*/
const replaceLodashEsRequire = (fileName) =>
replaceCode({
fileName,
replacements: {
"require('lodash-es')": "require('lodash')",
},
});
const getExternalModules = ({ dependencies, peerDependencies }) =>
Array.from(new Set([...Object.keys(dependencies ?? {}), ...Object.keys(peerDependencies ?? {})]));

/**
* Rollup configuration for generating the library `.js` bundle.
Expand All @@ -93,6 +63,7 @@ const replaceLodashEsRequire = (fileName) =>
*/
export const tsLibConfig = (pkg, inputFile, format = 'esm') => {
const buildMetadata = getBuildMetadata(pkg);
const externalModules = getExternalModules(pkg);

return {
input: inputFile,
Expand All @@ -101,7 +72,7 @@ export const tsLibConfig = (pkg, inputFile, format = 'esm') => {
format,
banner: getBanner(pkg, buildMetadata),
},
external: getExternalModuleRegExps(pkg),
external: externalModules.map((m) => new RegExp(`^${m}(\\/.+)*$`)),
plugins: [
nodeResolve(),
commonjs(),
Expand All @@ -113,9 +84,8 @@ export const tsLibConfig = (pkg, inputFile, format = 'esm') => {
include: ['src/**/*', '../common/src/**/*'],
noEmitOnError: true,
}),
...(format === 'cjs' ? [replaceLodashEsRequire('index.js')] : []),
writeJSONFile({
fileName: 'build-meta.json',
fileName: 'build-metadata.json',
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

value: buildMetadata,
}),
analyzer({
Expand All @@ -125,27 +95,3 @@ export const tsLibConfig = (pkg, inputFile, format = 'esm') => {
],
};
};

/**
* Rollup configuration for generating the library `.d.ts` bundle.
*
* @param {import('type-fest').PackageJson} pkg
* @param {string} inputFile
* @returns {import('rollup').RollupOptions}
*/
export const dtsLibConfig = (pkg, inputFile) => ({
input: inputFile,
output: {
file: 'dist/index.d.ts',
},
external: [...getExternalModuleRegExps(pkg), /\.css$/],
plugins: [
dts({
respectExternal: true,
}),
analyzer({
summaryOnly: true,
root: rootDir,
}),
],
});
27 changes: 0 additions & 27 deletions packages/common/rollup-plugins/replaceCode.js

This file was deleted.

1 change: 0 additions & 1 deletion packages/common/src/index.ts
@@ -1,6 +1,5 @@
export * from './errors/CustomError';
export * from './errors/ErrorWithCause';
export * from './types/common';
export * from './types/fetch';
export * from './utils/logger';
export * from './utils/objects';
2 changes: 1 addition & 1 deletion packages/common/src/types/common.ts
Expand Up @@ -29,7 +29,7 @@ export type ReplaceProperties<T, R> = {
*
* Utility type, probably never a reason to export.
*/
type Never<T> = {
export type Never<T> = {
[K in keyof T]?: never;
};

Expand Down
4 changes: 2 additions & 2 deletions packages/common/src/utils/logger.ts
@@ -1,12 +1,12 @@
import * as _ from 'lodash-es';

// eslint-disable-next-line @typescript-eslint/no-explicit-any
type LogFunction = (message?: any, ...optionalParams: any[]) => void;
export type LogFunction = (message?: any, ...optionalParams: any[]) => void;

/**
* Minimal logger interface.
*/
type Logger = Record<'info' | 'warn' | 'error', LogFunction>;
export type Logger = Record<'info' | 'warn' | 'error', LogFunction>;

const isProdEnv = process.env.NODE_ENV === 'production';

Expand Down
3 changes: 2 additions & 1 deletion packages/common/tsconfig-bases/lib-node-cjs.json
Expand Up @@ -5,6 +5,7 @@
"target": "es2021",
"module": "commonjs",
"moduleResolution": "node",
"declaration": true
"declaration": true,
"declarationMap": true
}
}
1 change: 1 addition & 0 deletions packages/common/tsconfig-bases/lib-react-esm.json
Expand Up @@ -6,6 +6,7 @@
"module": "esnext",
"moduleResolution": "node",
"declaration": true,
"declarationMap": true,
"jsx": "react"
}
}
19 changes: 19 additions & 0 deletions packages/eslint-plugin-internal/src/rules/all-bases.js
Expand Up @@ -16,4 +16,23 @@ module.exports = {

// Enforce a maximum number of classes per file
'max-classes-per-file': 'off',

// This rule has inconsistent behavior and poor monorepo support, replaced by lib-restricted-external-imports
'import/no-extraneous-dependencies': 'off',

// Enforce imported external modules to be declared in dependencies or peerDependencies within the closest parent package.json
'lib-restricted-external-imports': [
'error',
[
{
includeFiles: 'packages/lib-core/src/**',
excludeFiles: '**/*.test.*',
excludeModules: ['@monorepo/common'],
},
{
includeFiles: 'packages/+(lib-extensions|lib-utils|lib-webpack)/src/**',
excludeFiles: '**/*.+(test|stories).*',
},
],
],
};