From f39a36dc125caca7866eeb70b6d8ef6bc14a9095 Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 28 Oct 2021 19:48:59 -0400 Subject: [PATCH] Respect ignore patterns in FDirSourceFileProvider (#103) At present, FDirSourceFileProvider does not respect ignore patterns from the input tsconfig. Because of this, we run into issues in owa where, if you build packages in OWA then run good-fences with `-x`, you will get fence errors about non-exported members under `lib`. This change makes FDirSourceFileProvider aware of the ignore patterns in the passed in tsconfig (not recursively, however). As part of this change, I also - add an explicit dependency on @types/picomatch so we can check in our own code - turns on esModuleInterop so we can import picomatch directly - removes deprecated types from @types/commander, since commander ships with types now. - Updates the integration test to handle ignore patterns (in a very artificial way) - Updates the integration test to test the FDir provider, and normalize result orders (since result ordering is nondeterministic in the fdir provider). - Adds a way to clear results so you can run good-fences multiple times in the same process without accumulating errors. --- package.json | 2 +- .../ignored/ignoredFileWithIllegalImports.ts | 5 ++++ sample/tsconfig.json | 2 +- src/core/FdirSourceFileProvider.ts | 25 ++++++++++++---- src/core/cli.ts | 2 +- src/core/result.ts | 9 +++++- src/core/runner.ts | 8 +++-- test/endToEnd/endToEndTests.ts | 30 +++++++++++++++++++ test/utils/__mocks__/fs.ts | 12 ++++++++ test/utils/loadConfigTests.ts | 1 + tsconfig.json | 1 + yarn.lock | 14 ++++----- 12 files changed, 92 insertions(+), 19 deletions(-) create mode 100644 sample/src/componentB/ignored/ignoredFileWithIllegalImports.ts create mode 100644 test/utils/__mocks__/fs.ts diff --git a/package.json b/package.json index 7f0fc6f..0c57031 100644 --- a/package.json +++ b/package.json @@ -26,10 +26,10 @@ }, "devDependencies": { "@types/cli-progress": "^3.9.2", - "@types/commander": "^2.12.2", "@types/jest": "^26.0.15", "@types/node": "^12.7.8", "@types/nodegit": "^0.27.2", + "@types/picomatch": "^2.3.0", "husky": "^4.3.8", "jest": "^26.6.0", "prettier": "^2.2.1", diff --git a/sample/src/componentB/ignored/ignoredFileWithIllegalImports.ts b/sample/src/componentB/ignored/ignoredFileWithIllegalImports.ts new file mode 100644 index 0000000..8d75b84 --- /dev/null +++ b/sample/src/componentB/ignored/ignoredFileWithIllegalImports.ts @@ -0,0 +1,5 @@ +import helperA2 from '../../componentA/helperA2'; + +export default function ignoredComponent() { + helperA2(); +} diff --git a/sample/tsconfig.json b/sample/tsconfig.json index 2e575ee..8f424cf 100644 --- a/sample/tsconfig.json +++ b/sample/tsconfig.json @@ -8,6 +8,6 @@ "noImplicitAny": false, "noUnusedLocals": true }, - "exclude": ["lib"], + "exclude": ["lib", "src/**/ignored/**/*.ts"], "include": ["src/**/*"] } diff --git a/src/core/FdirSourceFileProvider.ts b/src/core/FdirSourceFileProvider.ts index 1395b42..9905305 100644 --- a/src/core/FdirSourceFileProvider.ts +++ b/src/core/FdirSourceFileProvider.ts @@ -14,9 +14,11 @@ import { ParsedCommandLine, preProcessFile, } from 'typescript'; +import picomatch from 'picomatch'; export class FDirSourceFileProvider implements SourceFileProvider { parsedCommandLine: ParsedCommandLine; + excludePatternsPicoMatchers: picomatch.Matcher[]; matchPath: MatchPathAsync; private sourceFileGlob: string; private extensionsToCheckDuringImportResolution: string[]; @@ -50,6 +52,16 @@ export class FDirSourceFileProvider implements SourceFileProvider { } ); + const baseUrl = this.parsedCommandLine.options.baseUrl ?? path.dirname(configFileName); + this.excludePatternsPicoMatchers = (this.parsedCommandLine.raw?.exclude ?? []).map( + (excludePattern: string) => { + const matcher = picomatch(excludePattern); + return (pathToCheck: string) => { + return matcher(path.relative(baseUrl, pathToCheck)); + }; + } + ); + this.sourceFileGlob = `**/*@(${getScriptFileExtensions({ // Derive these settings from the typescript project itself allowJs: this.parsedCommandLine.options.allowJs || false, @@ -77,10 +89,7 @@ export class FDirSourceFileProvider implements SourceFileProvider { includeDefinitions: true, }); - this.matchPath = createMatchPathAsync( - this.parsedCommandLine.options.baseUrl, - this.parsedCommandLine.options.paths - ); + this.matchPath = createMatchPathAsync(baseUrl, this.parsedCommandLine.options.paths ?? {}); } async getSourceFiles(searchRoots?: string[]): Promise { @@ -95,7 +104,13 @@ export class FDirSourceFileProvider implements SourceFileProvider { ) ); - return [...new Set(allRootsDiscoveredFiles.reduce((a, b) => a.concat(b), []))]; + return [ + ...new Set(allRootsDiscoveredFiles.reduce((a, b) => a.concat(b), [])), + ].filter((p: string) => !this.isPathExcluded(p)); + } + + private isPathExcluded(path: string) { + return this.excludePatternsPicoMatchers.some(isMatch => isMatch(path)); } async getImportsForFile(filePath: string): Promise { diff --git a/src/core/cli.ts b/src/core/cli.ts index 7d543b7..474adce 100644 --- a/src/core/cli.ts +++ b/src/core/cli.ts @@ -7,7 +7,7 @@ async function main() { const packageVersion = require('../../package.json').version; // Parse command line options - const program = commander + const program = commander.program .version(packageVersion) .option('-p, --project ', 'tsconfig.json file') .option('-r, --rootDir ', 'root directories of the project') diff --git a/src/core/result.ts b/src/core/result.ts index 88f6da6..a931d60 100644 --- a/src/core/result.ts +++ b/src/core/result.ts @@ -4,11 +4,18 @@ import GoodFencesError from '../types/GoodFencesError'; import GoodFencesResult from '../types/GoodFencesResult'; import ImportRecord from './ImportRecord'; -const result: GoodFencesResult = { +let result: GoodFencesResult = { errors: [], warnings: [], }; +export function resetResult() { + result = { + errors: [], + warnings: [], + }; +} + export function getResult() { return result; } diff --git a/src/core/runner.ts b/src/core/runner.ts index 2d42389..9519478 100644 --- a/src/core/runner.ts +++ b/src/core/runner.ts @@ -3,7 +3,7 @@ import getOptions, { setOptions } from '../utils/getOptions'; import validateFile from '../validation/validateFile'; import TypeScriptProgram from './TypeScriptProgram'; import normalizePath from '../utils/normalizePath'; -import { getResult, reportWarning } from './result'; +import { getResult, reportWarning, resetResult } from './result'; import { validateTagsExist } from '../validation/validateTagsExist'; import { SourceFileProvider } from './SourceFileProvider'; import { FDirSourceFileProvider } from './FdirSourceFileProvider'; @@ -98,5 +98,9 @@ export async function run(rawOptions: RawOptions) { options.progress ); - return getResult(); + const result = getResult(); + // Reset the global results object so so that future runs + // do not have the results from this run. + resetResult(); + return result; } diff --git a/test/endToEnd/endToEndTests.ts b/test/endToEnd/endToEndTests.ts index 8cad2ff..1ab1495 100644 --- a/test/endToEnd/endToEndTests.ts +++ b/test/endToEnd/endToEndTests.ts @@ -1,4 +1,5 @@ import { run } from '../../src/core/runner'; +import GoodFencesError from '../../src/types/GoodFencesError'; import GoodFencesResult from '../../src/types/GoodFencesResult'; import normalizePath from '../../src/utils/normalizePath'; @@ -15,6 +16,26 @@ describe('runner', () => { // Assert removeDetailedMessages(actualResults); normalizePaths(expectedResults); + normalizeOrder(actualResults); + normalizeOrder(expectedResults); + expect(actualResults).toEqual(expectedResults); + }); + + it('returns the expected results with looseRootFileDiscovery', async () => { + // Arrange + const expectedResults = require('./endToEndTests.expected.json'); + + // Act + const actualResults = await run({ + rootDir: './sample', + looseRootFileDiscovery: true, + }); + + // Assert + removeDetailedMessages(actualResults); + normalizePaths(expectedResults); + normalizeOrder(actualResults); + normalizeOrder(expectedResults); expect(actualResults).toEqual(expectedResults); }); }); @@ -29,6 +50,15 @@ function removeDetailedMessages(results: GoodFencesResult) { } } +function normalizeOrder(results: GoodFencesResult) { + results.errors.sort((a: GoodFencesError, b: GoodFencesError): number => + a.sourceFile.localeCompare(b.sourceFile) + ); + results.warnings.sort((a: GoodFencesError, b: GoodFencesError): number => + a.sourceFile.localeCompare(b.sourceFile) + ); +} + function normalizePaths(results: GoodFencesResult) { for (const error of results.errors) { error.fencePath = normalizePath(error.fencePath); diff --git a/test/utils/__mocks__/fs.ts b/test/utils/__mocks__/fs.ts new file mode 100644 index 0000000..63c53bf --- /dev/null +++ b/test/utils/__mocks__/fs.ts @@ -0,0 +1,12 @@ +// We mock the fs module here because in newer version of node, +// the builtin module epxorts are non-writeable. This means that +// spyOn(fs, 'readFileSync') will error in beforeEach and no mocks +// will actually be set. +// +// By providing a mock module here and calling jest.mock('fs') +// before any imports, we replace any imports of fs with +// this module, which has mutable exports. + +export function readFileSync() { + throw new Error('readFileSync mock was not overridden'); +} diff --git a/test/utils/loadConfigTests.ts b/test/utils/loadConfigTests.ts index 5dc9d96..98bafcc 100644 --- a/test/utils/loadConfigTests.ts +++ b/test/utils/loadConfigTests.ts @@ -1,3 +1,4 @@ +jest.mock('fs'); import * as fs from 'fs'; import RawConfig from '../../src/types/rawConfig/RawConfig'; import loadConfig, { normalizeExportRules } from '../../src/utils/loadConfig'; diff --git a/tsconfig.json b/tsconfig.json index f8b211f..ccbc8de 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -8,6 +8,7 @@ "noImplicitAny": true, "noUnusedLocals": true, "noImplicitThis": true, + "esModuleInterop": true, "skipLibCheck": true }, "exclude": ["node_modules", "lib"], diff --git a/yarn.lock b/yarn.lock index 03c6cfd..bd2e233 100644 --- a/yarn.lock +++ b/yarn.lock @@ -664,13 +664,6 @@ dependencies: "@types/node" "*" -"@types/commander@^2.12.2": - version "2.12.2" - resolved "https://registry.yarnpkg.com/@types/commander/-/commander-2.12.2.tgz#183041a23842d4281478fa5d23c5ca78e6fd08ae" - integrity sha512-0QEFiR8ljcHp9bAbWxecjVRuAMr16ivPiGOw6KFQBVrVd0RQIcM3xKdRisH2EDWgVWujiYtHwhSkSUoAAGzH7Q== - dependencies: - commander "*" - "@types/graceful-fs@^4.1.2": version "4.1.3" resolved "https://registry.yarnpkg.com/@types/graceful-fs/-/graceful-fs-4.1.3.tgz#039af35fe26bec35003e8d86d2ee9c586354348f" @@ -754,6 +747,11 @@ resolved "https://registry.yarnpkg.com/@types/parse-json/-/parse-json-4.0.0.tgz#2f8bb441434d163b35fb8ffdccd7138927ffb8c0" integrity sha512-//oorEZjL6sbPcKUaCdIGlIUeH26mgzimjBB77G6XRgnDl/L5wOnpyBGRe/Mmf5CVW3PwEBE1NjiMZ/ssFh4wA== +"@types/picomatch@^2.3.0": + version "2.3.0" + resolved "https://registry.yarnpkg.com/@types/picomatch/-/picomatch-2.3.0.tgz#75db5e75a713c5a83d5b76780c3da84a82806003" + integrity sha512-O397rnSS9iQI4OirieAtsDqvCj4+3eY1J+EPdNTKuHuRWIfUoGyzX294o8C4KJYaLqgSrd2o60c5EqCU8Zv02g== + "@types/prettier@^2.0.0": version "2.1.5" resolved "https://registry.yarnpkg.com/@types/prettier/-/prettier-2.1.5.tgz#b6ab3bba29e16b821d84e09ecfaded462b816b00" @@ -1318,7 +1316,7 @@ combined-stream@^1.0.6, combined-stream@~1.0.6: dependencies: delayed-stream "~1.0.0" -commander@*, commander@^7.2.0: +commander@^7.2.0: version "7.2.0" resolved "https://registry.yarnpkg.com/commander/-/commander-7.2.0.tgz#a36cb57d0b501ce108e4d20559a150a391d97ab7" integrity sha512-QrWXB+ZQSVPmIWIhtEO9H+gwHaMGYiF5ChvoJ+K9ZGHG/sVsa6yiesAD1GC/x46sET00Xlwo1u49RVVVzvcSkw==