plugin interface: internals

[chris48s]

Initial go at a plugin interface. More work to do here..

Next steps:

• Extend config file format to allow user to specify userPlugins
• ~Do you want to allow loading plugins using command line args, or is this config file only Data point: ESLint does allow loading plugins and rules on the command line. Could have a --plugins (plural to match --catalogs) array param.~ https://github.com/chris48s/v8r/pull/477#discussion_r1698937178
• More plugin hooks: ~What else do you need? Probably do output formatters next..~
• ~Dogfood it: Try implementing more internal functionality as plugins as you define more hooks~
• ~Edit package.json to make the main export BasePlugin~
• ~Spike implementing other stuff as plugins~ https://github.com/chris48s/v8r/pull/477#issuecomment-2256614508
• Test loading plugins from outside the repo installed as NPM package
• Other TODO comments
• Documentation
  • Interface + plugin tutorial
  • Semver compatibility (what other internal do you need to expose? Might be useful to allow plugins to access some internals like the logger module, for example)
• Tests explicitly covering plugin functionality

I reckon you can probably merge a lot of this as just changes to internals before you expose/document it as an end user facing feature.

[chris48s]

~Thought:~

~If I use this hook to add a new document format that we can parse, there is no way to register it as a valid format in config-schema.json because it is just a static file.~

~Similarly, if you imagine I implement a hook that allows me to define another output format (beyond text and json).~

~I think I either need to~

• ~completely abandon the concept of using a static schema to validate the config file~
• ~keep the schema, but get rid of enums in the schema and just take any string for those values, but maybe do some additional validation on top of that.~

~Maybe in addition to defining a document parser and/or output formatter, plugins also need to declare strings they want to add to the enum(s). In the case of the output formatter, I also need to be able to inject another option into the yargs format arg~

~I reckon implementing the existing text and json formatters as internal plugins will help define the shape of this.~

[chris48s]

~think about naming convention for plugins (think ESLint)~

[chris48s]

Plugin mood room (for architectures, docs, etc): ESLint, Webpack

[chris48s]

I can see other plugin hooks that might be useful to expose. However, I think this is the palce to stop pulling the thread. The first iteration of the plugin interface should expose only

• registerDocumentFormats
• parseDocument
• registerOutputFormats
• logSingleResult
• logAllResults

Then I can iterate from there.

[chris48s]

As a test, here's a version of the unfinished SARIF work from https://github.com/chris48s/v8r/pull/217 implemented as a plugin using the registerOutputFormats() and logAllResults() hooks. This is written in such a way it could live outside this codebase. The key thing here is resolving our way to v8r 's package.json using require.resolve("v8r"); instead of a relative path:

import { createRequire } from "module";
// TODO: once JSON modules is stable these requires could become imports
// https://nodejs.org/api/esm.html#esm_experimental_json_modules
const require = createRequire(import.meta.url);

import path from "path";
import url from "url";

import {
  SarifBuilder,
  SarifRunBuilder,
  SarifResultBuilder,
} from "node-sarif-builder";

import { BasePlugin } from "v8r";

class SarifOutput extends BasePlugin {
  static name = "v8r-plugin-sarif-output";

  registerOutputFormats() {
    return ["sarif"];
  }

  getAllResultsLogMessage(results, format) {
    if (format === "sarif") {
      const v8rMain = require.resolve("v8r");
      const pkg = require(
        path.join(path.parse(v8rMain).dir, "..", "package.json"),
      );

      const sarifBuilder = new SarifBuilder();
      const sarifRunBuilder = new SarifRunBuilder().initSimple({
        toolDriverName: pkg.name,
        toolDriverVersion: pkg.version,
        url: pkg.homepage,
      });
      for (const result of Object.values(results)) {
        if (result.valid) {
          const sarifResultBuilder = new SarifResultBuilder();
          const sarifResultInit = {
            level: "note",
            messageText: "file is valid",
            fileUri: url.pathToFileURL(result.fileLocation).href,
          };
          sarifResultBuilder.initSimple(sarifResultInit);
          sarifRunBuilder.addResult(sarifResultBuilder);
          continue;
        }

        for (const error of result.errors) {
          const sarifResultBuilder = new SarifResultBuilder();
          const sarifResultInit = {
            level: "error",
            ruleId: error.keyword,
            messageText: error.message,
            fileUri: url.pathToFileURL(result.fileLocation).href,
          };
          sarifResultBuilder.initSimple(sarifResultInit);
          sarifRunBuilder.addResult(sarifResultBuilder);
        }
      }
      sarifBuilder.addRun(sarifRunBuilder);
      const sarifJsonString = sarifBuilder.buildSarifJsonString({
        indent: true,
      });
      return sarifJsonString;
    }
  }
}

export default SarifOutput;

[chris48s]

~I think the next key thing to do is to allow loading userland plugins. This requires doing the big refactor of the config loader/bootstrap process.~

Then I think once you've done that its mostly:

• TODO comments
• unit tests
• remaining manual testing
• docs

[chris48s]

Question: What would be needed to get to a stage where you can merge this as an internals refactor without actually documenting the plugin interface yet? Is that a sensible next goal to aim for?

[chris48s]

OK, so I reckon to get this to a stage where you can merge this the things you need to sort out are:

• https://github.com/chris48s/v8r/blob/9bd0c53a1ca1b014db094b55f3dc2902c628c1cf/src/bootstrap.spec.js#L6-L9
• https://github.com/chris48s/v8r/blob/9bd0c53a1ca1b014db094b55f3dc2902c628c1cf/src/config-validators.spec.js#L38-L41
• Write some unit tests covering the new code you have written that is not under test.
• You also need to revisit the way you're marking plugins as core/user here: https://github.com/chris48s/v8r/blob/9bd0c53a1ca1b014db094b55f3dc2902c628c1cf/src/plugins.js#L82-L84
• You might also want to back out the changes to config-schema.json and make them later when you are documenting the feature as public.
• Delete https://github.com/chris48s/v8r/blob/9bd0c53a1ca1b014db094b55f3dc2902c628c1cf/src/public.js#L3-L10 and convert to a follow-up task
• Final pass of self-review + tidy up the commit history.

If you can do that, I reckon it makes sense to merge this PR just as an internal refactoring of how the existing document parser and output formats are implemented.

Then once you've done that, the follow-up tasks to launch it as a feature people can use are:

• Extend config file format to allow user to specify userPlugins https://github.com/chris48s/v8r/blob/9bd0c53a1ca1b014db094b55f3dc2902c628c1cf/src/bootstrap.js#L173-L174 (plus any additional automated tests needed to cover this)
• Test loading plugins from outside the repo installed as NPM package
• Documentation (this might require a bit of a restructure of the docs - you've probably outgrown a single markdown file at this point):
  • Interface + plugin tutorial
  • Semver compatibility statement
  • Add JSDoc comments to BasePlugin
  • If you reverted changes to config-schema.json, re-do them
  • Changelog
  • Anything else? General review

..and then tasks that are not necessary for minimum viable plugins but punt to issues for later are:

• https://github.com/chris48s/v8r/blob/9bd0c53a1ca1b014db094b55f3dc2902c628c1cf/src/public.js#L3-L10
• More plugin hooks: What else would be useful?

[chris48s]

OK. I reckon I'm going to leave the schema where it is.

I think everything else is done except

• ~tidy up the commit history~
• another pass of self-review
• ~can I think of better names for the 2 log* plugin methods?~

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 97.08738% with 12 lines in your changes missing coverage. Please review.

Project coverage is 95.96%. Comparing base (761f595) to head (1cd9bc0).

Files	Patch %	Lines
src/plugins.js	94.25%	5 Missing :warning:
src/plugins/parser-json.js	92.00%	2 Missing :warning:
src/plugins/parser-toml.js	90.90%	2 Missing :warning:
src/plugins/parser-yaml.js	90.90%	2 Missing :warning:
src/parser.js	91.66%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #477 +/- ## ========================================== + Coverage 95.39% 95.96% +0.56% ========================================== Files 11 20 +9 Lines 803 1139 +336 Branches 180 260 +80 ========================================== + Hits 766 1093 +327 - Misses 36 45 +9 Partials 1 1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[chris48s]

When you write up all the follow-up issues, add them to this milestone: https://github.com/chris48s/v8r/milestone/1