Conversion to TypeScript

[jamessimone]

TypeScript Conversion

Updates sfdx-scan-pull-request to use TypeScript

Rationale

1. Compile time safety:

There was a bug with the existing version of the action which TypeScript helps to expose:

// in JavaScript version, index.js
const TYPES_OF_INTEREST = new Set().add("add").add("delete");

becomes:

// in TypeScript version, git-actions.ts
if (change.type === "add" || change.type === "del") {

Note the subtle change from delete as a string, to del. It wasn't possible for a change returned from the parse-diff dependency to come back as delete, but only TypeScript can provide you with that sort of rich, compile-time error.

Additionally, because adding types to the action creates compile-time safety checks, it makes refactoring much easier. For example, adding support for the action to run on a designated target on push or PR ended up taking only a few minutes, because the vast majority of the architecture stays the same. Likewise, something like filtering existing comments to only those created by the action itself:

async function getExistingComments() {
  console.log("Getting existing comments using GitHub REST API...");
  return (await performGithubRequest<GithubExistingComment[]>("GET")).filter(
    (existingComment) => existingComment.user.type === "Bot"
  );
}

Becomes trivial, because all of the types in play are easy to define, take advantage of inheritance (GithubExistingComment descends from GithubComment); all of which helps to shift quality control left to the writing phase as opposed to the testing phase.

2. Bundle time optimizations:

The updated version of the action takes advantage of Vercel's excellent ncc package to do transpilation (though I had originally intended to use esbuild as the bundling dependency, there was a bug with GitHub's Octokit package that made transpilation there difficult due to transitive dependencies not being tree-shook correctly). This has one enormous benefit, in addition to converting the written TypeScript into CommonJS runnable by GitHub: it tree shakes the entire dependency tree, pulling your local dependencies into the transpiled version of the code. In layman's terms: it completely removes the onerous and ugly necessity of including all of your node_modules dependencies from the actual checked-in code, which makes the repo ... searchable, navigatable, performant to operate in, etc ...

Changes, Summarized

I've provided a brief summary on a function by function basis to help navigate the changes, since viewing the PR diff is actually impossible given the removal of node_modules. My advice is to view my version of the files with your original file in order to most easily make this comparison possible (going from top to bottom in the original file):

• initialSetup(): essentially stays the same. Minor changes overall to support the new target action property
• getGithubRestApiClient(): now part of performGithubRequest() in index.ts
• validatePullRequestContext(): becomes validateContext() in index.ts
• getExistingComments(): essentially the same, minor addition of filtering human-made comments
• getDiffInPullRequest(): moved to git-actions.ts, essentially the same despite the simple-git dependency
• recursivelyMoveFilesToTempFolder() removed, functionality in spirit moved to updateScannerTarget() by using comma-separated string
• performStaticCodeAnalysisOnFilesInDiff() - moved to sfdxCli.ts, no longer relies on output directory but instead parses output from SFDX. Note that the output is either a list of scanner violations or a string saying (essentially) "no violations found", so there's some additional post-function handling of the no-violation case
• filterFindingsToDiffScope() functionally very similar, uses JS Map instead of map-like object. Returns the hasHaltingError boolean now instead of mutating global state
• isInChangedLines() - the same
• translateViolationToComment(): functionally very similar, with the addition of the file as another column in the markdown table; returns the hasHaltingError boolean as well
• isHaltingViolation(): becomesgetScannerViolationType(), uses string literal enum type for strong typing support, otherwise identical
• writeComments(): has a bit more ceremony to bubble GitHub REST errors up to the action (since they're not a throw by default), otherwise functionally equivalent
• main(): better control flow to allow for parallel processing of asynchronous parts of the action

Development Lifecycle Changes

Note that there's now a pre-commit dependency on running the build command found in package.json. I didn't want to include a pre-commit hook in case you had a pre-existing preference for how to implement this, and the existing PR is ... big enough 😅.