From 3e27aefe10935fc34790c7a6e63c1e24f1e1e56b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CJamesHenry=E2=80=9D?= Date: Wed, 21 Sep 2022 20:48:19 +0200 Subject: [PATCH] fix(builder): remove nrwl/devkit from builder implementation --- .github/workflows/nx-migrate.yml | 3 - packages/builder/builders.json | 7 - packages/builder/package.json | 4 - packages/builder/src/compat.ts | 5 - packages/builder/src/lint.impl.spec.ts | 125 ++++++------ packages/builder/src/lint.impl.ts | 256 ++++++++++++------------ tools/scripts/update-builder-nx-deps.ts | 34 ---- yarn.lock | 4 +- 8 files changed, 190 insertions(+), 248 deletions(-) delete mode 100644 packages/builder/src/compat.ts delete mode 100644 tools/scripts/update-builder-nx-deps.ts diff --git a/.github/workflows/nx-migrate.yml b/.github/workflows/nx-migrate.yml index b0353c608..0e2ef10c5 100644 --- a/.github/workflows/nx-migrate.yml +++ b/.github/workflows/nx-migrate.yml @@ -59,9 +59,6 @@ jobs: npx nx migrate @nrwl/workspace@$NX_VERSION - # Update the version of nx packages specified in the builder package - NX_VERSION=$NX_VERSION npx ts-node --project ./tsconfig.tools.json ./tools/scripts/update-builder-nx-deps.ts - # Sometimes Nx can require config formatting changes after a migrate command yarn --ignore-scripts npx nx format diff --git a/packages/builder/builders.json b/packages/builder/builders.json index ebbf58489..d5ad20b7a 100644 --- a/packages/builder/builders.json +++ b/packages/builder/builders.json @@ -1,12 +1,5 @@ { "builders": { - "lint": { - "implementation": "./dist/compat", - "schema": "./dist/schema.json", - "description": "Run ESLint over a TypeScript project" - } - }, - "executors": { "lint": { "implementation": "./dist/lint.impl.js", "schema": "./dist/schema.json", diff --git a/packages/builder/package.json b/packages/builder/package.json index 0f20808b5..4907e26d6 100644 --- a/packages/builder/package.json +++ b/packages/builder/package.json @@ -18,10 +18,6 @@ "builders.json" ], "builders": "./builders.json", - "dependencies": { - "@nrwl/devkit": "^14.7.5", - "nx": "^14.7.5" - }, "peerDependencies": { "eslint": "^7.0.0 || ^8.0.0", "typescript": "*" diff --git a/packages/builder/src/compat.ts b/packages/builder/src/compat.ts deleted file mode 100644 index d04532c1b..000000000 --- a/packages/builder/src/compat.ts +++ /dev/null @@ -1,5 +0,0 @@ -import { convertNxExecutor } from '@nrwl/devkit'; - -import lintExecutor from './lint.impl'; - -export default convertNxExecutor(lintExecutor); diff --git a/packages/builder/src/lint.impl.spec.ts b/packages/builder/src/lint.impl.spec.ts index 61809b150..00d10713c 100644 --- a/packages/builder/src/lint.impl.spec.ts +++ b/packages/builder/src/lint.impl.spec.ts @@ -1,4 +1,6 @@ -import type { ExecutorContext } from '@nrwl/devkit'; +import { Architect } from '@angular-devkit/architect'; +import { TestingArchitectHost } from '@angular-devkit/architect/testing'; +import { json, logging, schema } from '@angular-devkit/core'; import type { ESLint } from 'eslint'; import { resolve } from 'path'; import type { Schema } from './schema'; @@ -49,7 +51,6 @@ jest.mock('./utils/eslint-utils', () => { ), }; }); -import lintExecutor from './lint.impl'; function createValidRunBuilderOptions( additionalOptions: Partial = {}, @@ -86,30 +87,42 @@ function setupMocks() { console.info = jest.fn(); } -describe('Linter Builder', () => { - let mockContext: ExecutorContext; +const loggerSpy = jest.fn(); + +async function runBuilder(options: Schema) { + const registry = new json.schema.CoreSchemaRegistry(); + registry.addPostTransform(schema.transforms.addUndefinedDefaults); + + const testArchitectHost = new TestingArchitectHost( + resolve('/root'), + resolve('/root'), + ); + const builderName = '@angular-eslint/builder:lint'; + /** + * Require in the implementation from src so that we don't need + * to run a build before tests run and it is dynamic enough + * to come after jest does its mocking + */ + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { default: builderImplementation } = require('./lint.impl'); + testArchitectHost.addBuilder(builderName, builderImplementation); + + const architect = new Architect(testArchitectHost, registry); + const logger = new logging.Logger(''); + logger.subscribe(loggerSpy); + + const run = await architect.scheduleBuilder(builderName, options, { + logger, + }); + + return run.result; +} + +describe('Linter Builder', () => { beforeEach(() => { MockESLint.version = VALID_ESLINT_VERSION; mockReports = [{ results: [], messages: [], usedDeprecatedRules: [] }]; - const projectName = 'proj'; - mockContext = { - projectName, - root: '/root', - cwd: '/root', - workspace: { - npmScope: '', - version: 2, - projects: { - [projectName]: { - root: `apps/${projectName}`, - sourceRoot: `apps/${projectName}/src`, - targets: {}, - }, - }, - }, - isVerbose: false, - }; }); afterAll(() => { @@ -119,7 +132,7 @@ describe('Linter Builder', () => { it('should throw if the eslint version is not supported', async () => { MockESLint.version = '1.6'; setupMocks(); - const result = lintExecutor(createValidRunBuilderOptions(), mockContext); + const result = runBuilder(createValidRunBuilderOptions()); await expect(result).rejects.toThrow( /ESLint must be version 7.6 or higher/, ); @@ -127,13 +140,13 @@ describe('Linter Builder', () => { it('should not throw if the eslint version is supported', async () => { setupMocks(); - const result = lintExecutor(createValidRunBuilderOptions(), mockContext); + const result = runBuilder(createValidRunBuilderOptions()); await expect(result).resolves.not.toThrow(); }); it('should invoke the linter with the options that were passed to the builder', async () => { setupMocks(); - await lintExecutor( + await runBuilder( createValidRunBuilderOptions({ lintFilePatterns: [], eslintConfig: './.eslintrc', @@ -153,7 +166,6 @@ describe('Linter Builder', () => { rulesdir: [], resolvePluginsRelativeTo: null, }), - mockContext, ); expect(mockLint).toHaveBeenCalledWith( resolve('/root'), @@ -183,35 +195,31 @@ describe('Linter Builder', () => { it('should throw if no reports generated', async () => { mockReports = []; setupMocks(); - const result = lintExecutor( - createValidRunBuilderOptions({ - lintFilePatterns: ['includedFile1'], - }), - mockContext, - ); - await expect(result).rejects.toThrow( - /Invalid lint configuration. Nothing to lint./, - ); + await expect( + runBuilder( + createValidRunBuilderOptions({ + lintFilePatterns: ['includedFile1'], + }), + ), + ).rejects.toThrow(/Invalid lint configuration. Nothing to lint./); }); it('should create a new instance of the formatter with the selected user option', async () => { setupMocks(); - await lintExecutor( + await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc', lintFilePatterns: ['includedFile1'], format: 'json', }), - mockContext, ); expect(mockLoadFormatter).toHaveBeenCalledWith('json'); - await lintExecutor( + await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc', lintFilePatterns: ['includedFile1'], format: 'html', }), - mockContext, ); expect(mockLoadFormatter).toHaveBeenCalledWith('html'); }); @@ -250,13 +258,12 @@ describe('Linter Builder', () => { }, ]; setupMocks(); - await lintExecutor( + await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc', lintFilePatterns: ['includedFile1', 'includedFile2'], format: 'checkstyle', }), - mockContext, ); expect(mockLoadFormatter).toHaveBeenCalledWith('checkstyle'); expect(mockFormatter.format).toHaveBeenCalledWith([ @@ -295,14 +302,13 @@ describe('Linter Builder', () => { it('should pass all the reports to the fix engine, even if --fix is false', async () => { setupMocks(); - await lintExecutor( + await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc', lintFilePatterns: ['includedFile1'], format: 'json', fix: false, }), - mockContext, ); expect(mockOutputFixes).toHaveBeenCalled(); }); @@ -326,14 +332,13 @@ describe('Linter Builder', () => { }, ]; setupMocks(); - await lintExecutor( + await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc', lintFilePatterns: ['includedFile1'], format: 'json', silent: false, }), - mockContext, ); expect(console.error).toHaveBeenCalledWith( 'Lint errors found in the listed files.\n', @@ -360,14 +365,13 @@ describe('Linter Builder', () => { }, ]; setupMocks(); - await lintExecutor( + await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc', lintFilePatterns: ['includedFile1'], format: 'json', silent: false, }), - mockContext, ); expect(console.error).not.toHaveBeenCalledWith( 'Lint errors found in the listed files.\n', @@ -395,7 +399,7 @@ describe('Linter Builder', () => { }, ]; setupMocks(); - await lintExecutor( + await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc', lintFilePatterns: ['includedFile1'], @@ -403,7 +407,6 @@ describe('Linter Builder', () => { maxWarnings: 1, silent: true, }), - mockContext, ); expect(console.error).not.toHaveBeenCalledWith( 'Lint errors found in the listed files.\n', @@ -438,7 +441,7 @@ describe('Linter Builder', () => { }, ]; setupMocks(); - await lintExecutor( + await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc', lintFilePatterns: ['includedFile1'], @@ -446,7 +449,6 @@ describe('Linter Builder', () => { quiet: true, silent: false, }), - mockContext, ); expect(console.info).not.toHaveBeenCalledWith('Mock warning message\n'); expect(console.error).not.toHaveBeenCalledWith( @@ -477,14 +479,13 @@ describe('Linter Builder', () => { }, ]; setupMocks(); - const output = await lintExecutor( + const output = await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc', lintFilePatterns: ['includedFile1'], format: 'json', silent: true, }), - mockContext, ); expect(output.success).toBeTruthy(); }); @@ -507,7 +508,7 @@ describe('Linter Builder', () => { }, ]; setupMocks(); - const output = await lintExecutor( + const output = await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc', lintFilePatterns: ['includedFile1'], @@ -515,7 +516,6 @@ describe('Linter Builder', () => { silent: true, force: true, }), - mockContext, ); expect(output.success).toBeTruthy(); }); @@ -538,7 +538,7 @@ describe('Linter Builder', () => { }, ]; setupMocks(); - const output = await lintExecutor( + const output = await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc', lintFilePatterns: ['includedFile1'], @@ -546,7 +546,6 @@ describe('Linter Builder', () => { silent: true, force: false, }), - mockContext, ); expect(output.success).toBeFalsy(); }); @@ -569,14 +568,13 @@ describe('Linter Builder', () => { }, ]; setupMocks(); - const output = await lintExecutor( + const output = await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc', lintFilePatterns: ['includedFile1'], format: 'json', maxWarnings: 5, }), - mockContext, ); expect(output.success).toBeFalsy(); }); @@ -592,7 +590,7 @@ describe('Linter Builder', () => { }, ]; setupMocks(); - await lintExecutor( + await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc', lintFilePatterns: ['includedFile1'], @@ -600,7 +598,6 @@ describe('Linter Builder', () => { quiet: true, maxWarnings: 0, }), - mockContext, ); expect(console.error).toHaveBeenCalledWith( 'Found 1 warnings, which exceeds your configured limit (0). Either increase your maxWarnings limit or fix some of the lint warnings.', @@ -625,7 +622,7 @@ describe('Linter Builder', () => { }, ]; setupMocks(); - await lintExecutor( + await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc.json', lintFilePatterns: ['includedFile1'], @@ -634,7 +631,6 @@ describe('Linter Builder', () => { force: false, outputFile: 'a/b/c/outputFile1', }), - mockContext, ); expect(mockCreateDirectory).toHaveBeenCalledWith('/root/a/b/c'); expect(fs.writeFileSync).toHaveBeenCalledWith( @@ -646,7 +642,7 @@ describe('Linter Builder', () => { it('should not attempt to write the lint results to the output file, if not specified', async () => { setupMocks(); jest.spyOn(fs, 'writeFileSync').mockImplementation(); - await lintExecutor( + await runBuilder( createValidRunBuilderOptions({ eslintConfig: './.eslintrc.json', lintFilePatterns: ['includedFile1'], @@ -654,7 +650,6 @@ describe('Linter Builder', () => { silent: true, force: false, }), - mockContext, ); expect(fs.writeFileSync).not.toHaveBeenCalled(); }); diff --git a/packages/builder/src/lint.impl.ts b/packages/builder/src/lint.impl.ts index ee42380d7..f79f16f19 100644 --- a/packages/builder/src/lint.impl.ts +++ b/packages/builder/src/lint.impl.ts @@ -1,4 +1,5 @@ -import type { ExecutorContext } from '@nrwl/devkit'; +import type { BuilderContext, BuilderOutput } from '@angular-devkit/architect'; +import { createBuilder } from '@angular-devkit/architect'; import type { ESLint } from 'eslint'; import { writeFileSync } from 'fs'; import { dirname, join, resolve } from 'path'; @@ -6,132 +7,131 @@ import type { Schema } from './schema'; import { createDirectory } from './utils/create-directory'; import { lint, loadESLint } from './utils/eslint-utils'; -export default async function run( - options: Schema, - context: ExecutorContext, -): Promise<{ success: boolean }> { - const workspaceRoot = context.root; - process.chdir(workspaceRoot); - - const projectName = context.projectName || ''; - const printInfo = options.format && !options.silent; - const reportOnlyErrors = options.quiet; - const maxWarnings = options.maxWarnings; - - if (printInfo) { - console.info(`\nLinting ${JSON.stringify(projectName)}...`); - } - - const projectESLint = await loadESLint(); - const version = projectESLint.ESLint?.version?.split('.'); - if ( - !version || - version.length < 2 || - Number(version[0]) < 7 || - (Number(version[0]) === 7 && Number(version[1]) < 6) - ) { - throw new Error('ESLint must be version 7.6 or higher.'); - } - - const eslint = new projectESLint.ESLint({}); - - /** - * We want users to have the option of not specifying the config path, and let - * eslint automatically resolve the `.eslintrc` files in each folder. - */ - const eslintConfigPath = options.eslintConfig - ? resolve(workspaceRoot, options.eslintConfig) - : undefined; - - const lintResults: ESLint.LintResult[] = await lint( - workspaceRoot, - eslintConfigPath, - options, - ); - - if (lintResults.length === 0) { - throw new Error('Invalid lint configuration. Nothing to lint.'); - } - - const formatter = await eslint.loadFormatter(options.format); - - let totalErrors = 0; - let totalWarnings = 0; - - // output fixes to disk, if applicable based on the options - await projectESLint.ESLint.outputFixes(lintResults); - - /** - * Depending on user configuration we may not want to report on all the - * results, so we need to adjust them before formatting. - */ - const finalLintResults: ESLint.LintResult[] = lintResults - .map((result): ESLint.LintResult | null => { - totalErrors += result.errorCount; - totalWarnings += result.warningCount; - - if (result.errorCount || (result.warningCount && !reportOnlyErrors)) { - if (reportOnlyErrors) { - // Collect only errors (Linter.Severity === 2) - result.messages = result.messages.filter( - ({ severity }) => severity === 2, - ); - } - - return result; - } - - return null; - }) - // Filter out the null values - .filter(Boolean) as ESLint.LintResult[]; - - const hasWarningsToPrint: boolean = totalWarnings > 0 && !reportOnlyErrors; - const hasErrorsToPrint: boolean = totalErrors > 0; - - /** - * It's important that we format all results together so that custom - * formatters, such as checkstyle, can provide a valid output for the - * whole project being linted. - * - * Additionally, apart from when outputting to a file, we want to always - * log (even when no results) because different formatters handled the - * "no results" case differently. - */ - const formattedResults = await formatter.format(finalLintResults); - - if (options.outputFile) { - const pathToOutputFile = join(context.root, options.outputFile); - createDirectory(dirname(pathToOutputFile)); - writeFileSync(pathToOutputFile, formattedResults); - } else { - console.info(formattedResults); - } - - if (hasWarningsToPrint && printInfo) { - console.warn('Lint warnings found in the listed files.\n'); - } - - if (hasErrorsToPrint && printInfo) { - console.error('Lint errors found in the listed files.\n'); - } - - if ( - (totalWarnings === 0 || reportOnlyErrors) && - totalErrors === 0 && - printInfo - ) { - console.info('All files pass linting.\n'); - } - - const tooManyWarnings = maxWarnings >= 0 && totalWarnings > maxWarnings; - if (tooManyWarnings && printInfo) { - console.error( - `Found ${totalWarnings} warnings, which exceeds your configured limit (${options.maxWarnings}). Either increase your maxWarnings limit or fix some of the lint warnings.`, +export default createBuilder( + async (options: Schema, context: BuilderContext): Promise => { + const workspaceRoot = context.workspaceRoot; + process.chdir(workspaceRoot); + + const projectName = context.target?.project || ''; + const printInfo = options.format && !options.silent; + const reportOnlyErrors = options.quiet; + const maxWarnings = options.maxWarnings; + + if (printInfo) { + console.info(`\nLinting ${JSON.stringify(projectName)}...`); + } + + const projectESLint = await loadESLint(); + const version = projectESLint.ESLint?.version?.split('.'); + if ( + !version || + version.length < 2 || + Number(version[0]) < 7 || + (Number(version[0]) === 7 && Number(version[1]) < 6) + ) { + throw new Error('ESLint must be version 7.6 or higher.'); + } + + const eslint = new projectESLint.ESLint({}); + + /** + * We want users to have the option of not specifying the config path, and let + * eslint automatically resolve the `.eslintrc` files in each folder. + */ + const eslintConfigPath = options.eslintConfig + ? resolve(workspaceRoot, options.eslintConfig) + : undefined; + + const lintResults: ESLint.LintResult[] = await lint( + workspaceRoot, + eslintConfigPath, + options, ); - } - return { - success: options.force || (totalErrors === 0 && !tooManyWarnings), - }; -} + if (lintResults.length === 0) { + throw new Error('Invalid lint configuration. Nothing to lint.'); + } + + const formatter = await eslint.loadFormatter(options.format); + + let totalErrors = 0; + let totalWarnings = 0; + + // output fixes to disk, if applicable based on the options + await projectESLint.ESLint.outputFixes(lintResults); + + /** + * Depending on user configuration we may not want to report on all the + * results, so we need to adjust them before formatting. + */ + const finalLintResults: ESLint.LintResult[] = lintResults + .map((result): ESLint.LintResult | null => { + totalErrors += result.errorCount; + totalWarnings += result.warningCount; + + if (result.errorCount || (result.warningCount && !reportOnlyErrors)) { + if (reportOnlyErrors) { + // Collect only errors (Linter.Severity === 2) + result.messages = result.messages.filter( + ({ severity }) => severity === 2, + ); + } + + return result; + } + + return null; + }) + // Filter out the null values + .filter(Boolean) as ESLint.LintResult[]; + + const hasWarningsToPrint: boolean = totalWarnings > 0 && !reportOnlyErrors; + const hasErrorsToPrint: boolean = totalErrors > 0; + + /** + * It's important that we format all results together so that custom + * formatters, such as checkstyle, can provide a valid output for the + * whole project being linted. + * + * Additionally, apart from when outputting to a file, we want to always + * log (even when no results) because different formatters handled the + * "no results" case differently. + */ + const formattedResults = await formatter.format(finalLintResults); + + if (options.outputFile) { + const pathToOutputFile = join(context.workspaceRoot, options.outputFile); + createDirectory(dirname(pathToOutputFile)); + writeFileSync(pathToOutputFile, formattedResults); + } else { + console.info(formattedResults); + } + + if (hasWarningsToPrint && printInfo) { + console.warn('Lint warnings found in the listed files.\n'); + } + + if (hasErrorsToPrint && printInfo) { + console.error('Lint errors found in the listed files.\n'); + } + + if ( + (totalWarnings === 0 || reportOnlyErrors) && + totalErrors === 0 && + printInfo + ) { + console.info('All files pass linting.\n'); + } + + const tooManyWarnings = maxWarnings >= 0 && totalWarnings > maxWarnings; + if (tooManyWarnings && printInfo) { + console.error( + `Found ${totalWarnings} warnings, which exceeds your configured limit (${options.maxWarnings}). Either increase your maxWarnings limit or fix some of the lint warnings.`, + ); + } + + return { + success: options.force || (totalErrors === 0 && !tooManyWarnings), + }; + }, +); diff --git a/tools/scripts/update-builder-nx-deps.ts b/tools/scripts/update-builder-nx-deps.ts deleted file mode 100644 index 8eb8ba899..000000000 --- a/tools/scripts/update-builder-nx-deps.ts +++ /dev/null @@ -1,34 +0,0 @@ -import { writeFileSync } from 'fs'; -import { join } from 'path'; -import { format, resolveConfig } from 'prettier'; - -/** - * Used within the nx-migrate.yml workflow to help automate renovate PRs - */ -(async function updateBuilderNxDeps() { - try { - if (!process.env.NX_VERSION || !process.env.NX_VERSION.length) { - throw new Error('NX_VERSION environment variable is not set'); - } - const packageJsonPath = join( - __dirname, - '../../packages/builder/package.json', - ); - // eslint-disable-next-line @typescript-eslint/no-var-requires - const packageJson = require(packageJsonPath); - const finalVersion = `^${process.env.NX_VERSION}`; - packageJson.dependencies['@nrwl/devkit'] = finalVersion; - packageJson.dependencies['nx'] = finalVersion; - - writeFileSync( - packageJsonPath, - format(JSON.stringify(packageJson, null, 2), { - ...(await resolveConfig(packageJsonPath)), - parser: 'json', - }), - ); - } catch (err) { - console.error(err); - process.exit(1); - } -})(); diff --git a/yarn.lock b/yarn.lock index 2a586af0c..c8faa6eea 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3093,7 +3093,7 @@ dependencies: nx "14.7.5" -"@nrwl/devkit@14.7.5", "@nrwl/devkit@^14.7.5": +"@nrwl/devkit@14.7.5": version "14.7.5" resolved "https://registry.yarnpkg.com/@nrwl/devkit/-/devkit-14.7.5.tgz#18e476b2050ca00fe8fcb34f2d988e94dc2b9d4b" integrity sha512-r0G5xhC48O8YPw+9jRVLxpXM7DadBWtS4pH1GeAAKgqlZloSpT4pZpHTqXH0z2h9S1EHcdtpSlRqzTe+PBUaRQ== @@ -9972,7 +9972,7 @@ nx@14.6.5, "nx@>=14.6.1 < 16": yargs "^17.4.0" yargs-parser "21.0.1" -nx@14.7.5, nx@^14.7.5: +nx@14.7.5: version "14.7.5" resolved "https://registry.yarnpkg.com/nx/-/nx-14.7.5.tgz#29b24560ebbd29c68b316ee52be90c9b9c2be12d" integrity sha512-hp8TYk/t15MJVXQCafSduriZqoxR2zvw5mDHqg32Mjt2jFEFKaPWtaO5l/qKj+rlLE8cPYTeGL5qAS9WZkAWtg==