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

build: allow auto-discover all typings files in npm package by ts-api-guardian #35691

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
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
60 changes: 60 additions & 0 deletions tools/ts-api-guardian/index.bzl
Expand Up @@ -74,3 +74,63 @@ def ts_api_guardian_test(
templated_args = args + ["--out", golden, actual],
**kwargs
)

def ts_api_guardian_test_npm_package(
name,
goldenDir,
actualDir,
data = [],
strip_export_pattern = ["^__", "^ɵ[^ɵ]"],
allow_module_identifiers = COMMON_MODULE_IDENTIFIERS,
use_angular_tag_rules = True,
**kwargs):
"""Runs ts_api_guardian
"""
data += [
# Locally we need to add the TS build target
# But it will replaced to @npm//ts-api-guardian when publishing
"@angular//tools/ts-api-guardian:lib",
"@angular//tools/ts-api-guardian:bin",
# The below are required during runtime
"@npm//chalk",
"@npm//diff",
"@npm//minimist",
"@npm//typescript",
]

args = [
# Needed so that node doesn't walk back to the source directory.
# From there, the relative imports would point to .ts files.
"--node_options=--preserve-symlinks",
# We automatically discover the enpoints for our NPM package.
"--autoDiscoverEntrypoints",
]

for i in strip_export_pattern:
# The below replacement is needed because under Windows '^' needs to be escaped twice
args += ["--stripExportPattern", i.replace("^", "^^^^")]

for i in allow_module_identifiers:
args += ["--allowModuleIdentifiers", i]

if use_angular_tag_rules:
args += ["--useAngularTagRules"]

nodejs_test(
name = name,
data = data,
entry_point = "@angular//tools/ts-api-guardian:bin/ts-api-guardian",
templated_args = args + ["--verifyDir", goldenDir, "--rootDir", actualDir],
tags = ["api_guard"],
**kwargs
)

nodejs_binary(
name = name + ".accept",
testonly = True,
data = data,
entry_point = "@angular//tools/ts-api-guardian:bin/ts-api-guardian",
templated_args = args + ["--outDir", goldenDir, "--rootDir", actualDir],
tags = ["api_guard"],
**kwargs
)
64 changes: 46 additions & 18 deletions tools/ts-api-guardian/lib/cli.ts
Expand Up @@ -14,7 +14,7 @@ const chalk = require('chalk');
import * as minimist from 'minimist';
import * as path from 'path';

import {SerializationOptions, generateGoldenFile, verifyAgainstGoldenFile} from './main';
import {SerializationOptions, generateGoldenFile, verifyAgainstGoldenFile, discoverAllEntrypoints} from './main';

const CMD = 'ts-api-guardian';

Expand Down Expand Up @@ -46,14 +46,23 @@ export function startCli() {
};
}

// In autoDiscoverEntrypoints mode we set the inputed files as the discovered entrypoints
// for the rootDir
let entrypoints: string[];
if (argv['autoDiscoverEntrypoints']) {
entrypoints = discoverAllEntrypoints(argv['rootDir']);
} else {
entrypoints = argv._.slice();
}

devversion marked this conversation as resolved.
Show resolved Hide resolved
for (const error of errors) {
console.warn(error);
}

if (mode === 'help') {
printUsageAndExit(!!errors.length);
} else {
const targets = resolveFileNamePairs(argv, mode);
const targets = resolveFileNamePairs(argv, mode, entrypoints);

if (mode === 'out') {
for (const {entrypoint, goldenFile} of targets) {
Expand Down Expand Up @@ -110,7 +119,7 @@ export function parseArguments(input: string[]):
'allowModuleIdentifiers'
],
boolean: [
'help', 'useAngularTagRules',
'help', 'useAngularTagRules', 'autoDiscoverEntrypoints',
// Options used by chalk automagically
'color', 'no-color'
],
Expand Down Expand Up @@ -147,15 +156,26 @@ export function parseArguments(input: string[]):
modes.push('verify');
}

if (!argv._.length) {
errors.push('No input file specified.');
modes = ['help'];
} else if (modes.length !== 1) {
errors.push('Specify either --out[Dir] or --verify[Dir]');
modes = ['help'];
} else if (argv._.length > 1 && !argv['outDir'] && !argv['verifyDir']) {
errors.push(`More than one input specified. Use --${modes[0]}Dir instead.`);
modes = ['help'];
if (argv['autoDiscoverEntrypoints']) {
if (!argv['rootDir']) {
errors.push(`--rootDir must be provided with --autoDiscoverEntrypoints.`);
modes = ['help'];
}
if (!argv['outDir'] && !argv['verifyDir']) {
errors.push(`--outDir or --verifyDir must be used with --autoDiscoverEntrypoints.`);
modes = ['help'];
}
} else {
if (!argv._.length) {
errors.push('No input file specified.');
modes = ['help'];
} else if (modes.length !== 1) {
errors.push('Specify either --out[Dir] or --verify[Dir]');
modes = ['help'];
} else if (argv._.length > 1 && !argv['outDir'] && !argv['verifyDir']) {
errors.push(`More than one input specified. Use --${modes[0]}Dir instead.`);
modes = ['help'];
}
}

return {argv, mode: modes[0], errors};
Expand Down Expand Up @@ -184,7 +204,8 @@ Options:
--useAngularTagRules <boolean> Whether the Angular specific tag rules should be used.
--stripExportPattern <regexp> Do not output exports matching the pattern
--allowModuleIdentifiers <identifier>
Allow identifier for "* as foo" imports`);
Allow identifier for "* as foo" imports
--autoDiscoverEntrypoints Automatically find all entrypoints .d.ts files in the rootDir`);
process.exit(error ? 1 : 0);
}

Expand All @@ -199,24 +220,31 @@ function resolveBazelFilePath(fileName: string): string {
// are not available in the working directory. In order to resolve the real path for the
// runfile, we need to use `require.resolve` which handles runfiles properly on Windows.
if (process.env['BAZEL_TARGET']) {
return path.relative(process.cwd(), require.resolve(fileName));
// This try/catch block is necessary because if the path is to the source file directly
// rather than via symlinks in the bazel output directories, require is not able to
// resolve it.
try {
return path.relative(process.cwd(), require.resolve(fileName));
} catch (err) {
return path.relative(process.cwd(), fileName);
}
}

return fileName;
}

function resolveFileNamePairs(
argv: minimist.ParsedArgs, mode: string): {entrypoint: string, goldenFile: string}[] {
function resolveFileNamePairs(argv: minimist.ParsedArgs, mode: string, entrypoints: string[]):
{entrypoint: string, goldenFile: string}[] {
if (argv[mode]) {
return [{
entrypoint: resolveBazelFilePath(argv._[0]),
entrypoint: resolveBazelFilePath(entrypoints[0]),
goldenFile: resolveBazelFilePath(argv[mode]),
}];
} else { // argv[mode + 'Dir']
let rootDir = argv['rootDir'] || '.';
const goldenDir = argv[mode + 'Dir'];

return argv._.map((fileName: string) => {
return entrypoints.map((fileName: string) => {
return {
entrypoint: resolveBazelFilePath(fileName),
goldenFile: resolveBazelFilePath(path.join(goldenDir, path.relative(rootDir, fileName))),
Expand Down
56 changes: 55 additions & 1 deletion tools/ts-api-guardian/lib/main.ts
Expand Up @@ -16,14 +16,22 @@ export {SerializationOptions, publicApi} from './serializer';
export function generateGoldenFile(
entrypoint: string, outFile: string, options: SerializationOptions = {}): void {
const output = publicApi(entrypoint, options);

// BUILD_WORKSPACE_DIRECTORY environment variable is only available during bazel
// run executions. This workspace directory allows us to generate golden files directly
// in the source file tree rather than via a symlink.
if (process.env['BUILD_WORKSPACE_DIRECTORY']) {
outFile = path.join(process.env['BUILD_WORKSPACE_DIRECTORY'], outFile);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this is a symptom from an issue where the golden file does not yet exist, so require.resolve (runfile resolution) won't work? I think we should respect this environment variable in the resolveBazelFilePath variable if the runfile resolution fails. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

BUILD_WORKSPACE_DIRECTORY will only be available during bazel run, so it only really comes into play for us when we are attempting to generate and write the file.

During a bazel test this environment variable is not available, so we would have to fallback anyway in this case inside of the resolveBazelFilePath function.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense. Optional: Add a comment for this here, and maybe also add a comment in resolveBazelFilePath that the try/catch is needed for resolution of files within tree artifacts?

You might able to just make this a noop-catch and return fileName; should do the same. I think we needed path.relative only for the absolute resolved path from the manifest.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in the comments.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding these! Could we make those more concrete/helpful by saying that:

In resolveBazelFilePath: We need the try/catch to handle file resolution for tree artifacts where individual files inside the tree artifact (e.g. npm_package) are not declared outputs and cannot be resolved through the manifest?

The BUILD_WORKSPACE_DIRECTORY variable is needed only for approving the golden in case the golden is not existing and the golden file is not symlinked/part of the manifest. Right?


ensureDirectory(path.dirname(outFile));
fs.writeFileSync(outFile, output);
}

export function verifyAgainstGoldenFile(
entrypoint: string, goldenFile: string, options: SerializationOptions = {}): string {
const actual = publicApi(entrypoint, options);
const expected = fs.readFileSync(goldenFile).toString();
const expected = fs.existsSync(goldenFile) ? fs.readFileSync(goldenFile).toString() : '';
devversion marked this conversation as resolved.
Show resolved Hide resolved

if (actual === expected) {
return '';
Expand All @@ -43,3 +51,49 @@ function ensureDirectory(dir: string) {
fs.mkdirSync(dir);
}
}

/**
* Determine if the provided path is a directory.
*/
function isDirectory(dirPath: string) {
try {
fs.lstatSync(dirPath).isDirectory();
} catch {
return false;
}
}

/**
* Gets an array of paths to the typings files for each of the recursively discovered
* package.json
* files from the directory provided.
*/
devversion marked this conversation as resolved.
Show resolved Hide resolved
export function discoverAllEntrypoints(dirPath: string) {
// Determine all of the package.json files
const packageJsons: string[] = [];
const entryPoints: string[] = [];
const findPackageJsonsInDir = (nextPath: string) => {
for (const file of fs.readdirSync(nextPath)) {
const fullPath = path.join(nextPath, file);
if (isDirectory(fullPath)) {
findPackageJsonsInDir(fullPath);
} else {
if (file === 'package.json') {
packageJsons.push(fullPath);
}
}
}
};
findPackageJsonsInDir(dirPath);

// Get all typings file locations from package.json files
for (const packageJson of packageJsons) {
const packageJsonObj = JSON.parse(fs.readFileSync(packageJson, {encoding: 'utf8'}));
const typings = packageJsonObj.typings;
if (typings) {
entryPoints.push(path.join(path.dirname(packageJson), typings));
}
}

return entryPoints;
}
18 changes: 18 additions & 0 deletions tools/ts-api-guardian/test/cli_unit_test.ts
Expand Up @@ -62,4 +62,22 @@ describe('cli: parseArguments', () => {
chai.assert.equal(mode, 'verify');
chai.assert.deepEqual(errors, []);
});

it('should show usage with error when supplied with --autoDiscoverEntrypoints without --baseDir',
() => {
const {mode, errors} =
parseArguments(['--autoDiscoverEntrypoints', '--outDir', 'something']);
chai.assert.equal(mode, 'help');
chai.assert.deepEqual(
errors, ['--rootDir must be provided with --autoDiscoverEntrypoints.']);
});

it('should show usage with error when supplied with --autoDiscoverEntrypoints without --outDir/verifyDir',
() => {
const {mode, errors} =
parseArguments(['--autoDiscoverEntrypoints', '--rootDir', 'something']);
chai.assert.equal(mode, 'help');
chai.assert.deepEqual(
errors, ['--outDir or --verifyDir must be used with --autoDiscoverEntrypoints.']);
});
});