ci: add and use linting rule to check /help URLs (#17936)

.eslintrc.js

@@ -31,9 +31,11 @@ const config = {
       },
     ],
   },
+  plugins: ['@sourcegraph/sourcegraph'],
   rules: {
     // Rules that are specific to this repo
     // All other rules should go into https://github.com/sourcegraph/eslint-config
+    '@sourcegraph/sourcegraph/check-help-links': 'error',
     'no-restricted-imports': [
       'error',
       {

client/packages/@sourcegraph/eslint-plugin-sourcegraph/.gitignore

@@ -0,0 +1 @@
+yarn.lock

client/packages/@sourcegraph/eslint-plugin-sourcegraph/README.md

@@ -0,0 +1,30 @@
+# eslint-plugin-sourcegraph
+
+Custom ESLint rules for Sourcegraph. This package should only be used within
+the main Sourcegraph project, and isn't intended for reuse by other packages in
+the Sourcegraph organisation.
+
+## Rules
+
+Rules are defined in `lib/rules`. At present, one rule is available.
+
+### `check-help-links`
+
+This rule parses `Link` and `a` elements in JSX/TSX files. If a list of valid
+docsite pages is provided, elements that point to a `/help/*` link are checked
+against that list: if they don't exist, a linting error is raised.
+
+The list of docsite pages is provided either via the `DOCSITE_LIST` environment
+variable, which should be a newline separated list of pages as outputted by
+`docsite ls`, or via the `docsiteList` rule option, which is the same data as
+an array.
+
+If neither of these are set, then the rule will silently succeed.
+
+## Testing
+
+Unit tests can be run with:
+
+```sh
+yarn test
+```

client/packages/@sourcegraph/eslint-plugin-sourcegraph/lib/index.js

@@ -0,0 +1,14 @@
+'use strict'
+
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+var requireIndex = require('requireindex')
+
+//------------------------------------------------------------------------------
+// Plugin Definition
+//------------------------------------------------------------------------------
+
+// import all rules in lib/rules
+module.exports.rules = requireIndex(__dirname + '/rules')

client/packages/@sourcegraph/eslint-plugin-sourcegraph/lib/rules/check-help-links.js

@@ -0,0 +1,101 @@
+'use strict'
+
+//------------------------------------------------------------------------------
+// Rule Definition
+//------------------------------------------------------------------------------
+
+module.exports = {
+  meta: {
+    docs: {
+      description: 'Check that /help links point to real, non-redirected pages',
+      category: 'Best Practices',
+      recommended: false,
+    },
+    schema: [
+      {
+        type: 'object',
+        properties: {
+          docsiteList: {
+            type: 'array',
+            items: {
+              type: 'string',
+            },
+          },
+        },
+        additionalProperties: false,
+      },
+    ],
+  },
+
+  create: function (context) {
+    // Build the set of valid pages. In order, we'll try to get this from:
+    //
+    // 1. The DOCSITE_LIST environment variable, which should be a newline
+    //    separated list of pages, as outputted by `docsite ls`.
+    // 2. The docsiteList rule option, which should be an array of pages.
+    //
+    // If neither of these are set, this rule will silently pass, so as not to
+    // require docsite to be run when a user wants to run eslint in general.
+    const pages = new Set()
+    if (process.env.DOCSITE_LIST) {
+      process.env.DOCSITE_LIST.split('\n').forEach(page => pages.add(page))
+    } else if (context.options.length > 0) {
+      context.options[0].docsiteList.forEach(page => pages.add(page))
+    }
+
+    // No pages were provided, so we'll return an empty object and do nothing.
+    if (pages.size === 0) {
+      return {}
+    }
+
+    // Return the object that will install the listeners we want. In this case,
+    // we only need to look at JSX opening elements.
+    //
+    // Note that we could use AST selectors below, but the structure of the AST
+    // makes that tricky: the identifer (Link or a) and attribute (to or href)
+    // we use to identify an element of interest are siblings, so we'd probably
+    // have to select on the identifier and have some ugly traversal code below
+    // to check the attribute. It feels cleaner to do it this way with the
+    // opening element as the context.
+    return {
+      JSXOpeningElement: node => {
+        // Figure out what kind of element we have and therefore what attribute
+        // we'd want to look for.
+        let attrName
+        if (node.name.name === 'Link') {
+          attrName = 'to'
+        } else if (node.name.name === 'a') {
+          attrName = 'href'
+        } else {
+          // Anything that's not a link is uninteresting.
+          return
+        }
+
+        // Go find the link target in the attribute array.
+        const target = node.attributes.reduce(
+          (target, attr) => target ?? (attr.name && attr.name.name === attrName ? attr.value.value : undefined),
+          undefined
+        )
+
+        // Make sure the target points to a help link; if not, we don't need to
+        // go any further.
+        if (!target || !target.startsWith('/help/')) {
+          return
+        }
+
+        // Strip off the /help/ prefix, any anchor, and any trailing slash, then
+        // look up the resultant page in the pages set, bearing in mind that it
+        // might point to a directory and we also need to look for any index
+        // page that might exist.
+        const destination = target.substring(6).split('#')[0].replace(/\/+$/, '')
+        if (!pages.has(destination + '.md') && !pages.has(destination + '/index.md')) {
+          context.report({
+            node,
+            message: 'Help link to non-existent page: {{ destination }}',
+            data: { destination },
+          })
+        }
+      },
+    }
+  },
+}

client/packages/@sourcegraph/eslint-plugin-sourcegraph/package.json

@@ -0,0 +1,22 @@
+{
+  "name": "eslint-plugin-sourcegraph",
+  "version": "0.0.0",
+  "description": "Custom ESLint rules for Sourcegraph",
+  "keywords": ["eslint", "eslintplugin", "eslint-plugin"],
+  "main": "lib/index.js",
+  "scripts": {
+    "eslint": "../../../../node_modules/.bin/eslint",
+    "test": "mocha tests --recursive"
+  },
+  "dependencies": {
+    "requireindex": "~1.1.0"
+  },
+  "devDependencies": {
+    "eslint": "^7.1.0",
+    "mocha": "^7.2.0"
+  },
+  "engines": {
+    "node": ">=0.10.0"
+  },
+  "license": "Apache-2.0"
+}

client/packages/@sourcegraph/eslint-plugin-sourcegraph/tests/lib/rules/check-help-links.test.js

@@ -0,0 +1,81 @@
+'use strict'
+
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+var rule = require('../../../lib/rules/check-help-links'),
+  RuleTester = require('eslint').RuleTester
+
+// Set up the configuration such that JSX is valid.
+RuleTester.setDefaultConfig({
+  parserOptions: {
+    ecmaVersion: 6,
+    ecmaFeatures: {
+      jsx: true,
+    },
+  },
+})
+
+//------------------------------------------------------------------------------
+// Tests
+//------------------------------------------------------------------------------
+
+const ruleTester = new RuleTester()
+const invalidLinkError = path => {
+  return { message: 'Help link to non-existent page: ' + path, type: 'JSXOpeningElement' }
+}
+const options = [{ docsiteList: ['a.md', 'b/c.md', 'd/index.md'] }]
+
+// Build up the test cases given the various combinations we need to support.
+const cases = { valid: [], invalid: [] }
+for (const [element, attribute] of [
+  ['a', 'href'],
+  ['Link', 'to'],
+]) {
+  for (const anchor of ['', '#anchor', '#anchor#double']) {
+    for (const content of ['', 'link content']) {
+      const code = target =>
+        content
+          ? `<${element} ${attribute}="${target}${anchor}">${content}</${element}>`
+          : `<${element} ${attribute}="${target}${anchor}" />`
+
+      cases.valid.push(
+        ...[
+          '/help/a',
+          '/help/b/c',
+          '/help/d',
+          '/help/d/',
+          'not-a-help-link',
+          'help/but-not-absolute',
+          '/help-but-not-a-directory',
+        ].map(target => {
+          return {
+            code: code(target),
+            options,
+          }
+        })
+      )
+
+      cases.invalid.push(
+        ...['/help/', '/help/b', '/help/does/not/exist'].map(target => {
+          return {
+            code: code(target),
+            errors: [invalidLinkError(target.substring(6))],
+            options,
+          }
+        })
+      )
+    }
+  }
+}
+
+// Every case should be valid if the options are empty.
+cases.valid.push(
+  ...[...cases.invalid, ...cases.valid].map(({ code }) => {
+    return { code }
+  })
+)
+
+// Actually run the tests.
+ruleTester.run('check-help-links', rule, cases)

dev/foreach-ts-project.sh

@@ -16,6 +16,7 @@ DIRS=(
   client/branded
   client/browser
   client/packages/sourcegraph-extension-api
+  client/packages/@sourcegraph/eslint-plugin-sourcegraph
   client/packages/@sourcegraph/extension-api-types
   dev/release
   dev/ts-morph

package.json

@@ -13,7 +13,7 @@
   "scripts": {
     "prettier": "prettier '**/{*.{js?(on),ts?(x),graphql,md,scss},.*.js?(on)}' --write --list-different --config prettier.config.js",
     "prettier-check": "yarn -s run prettier --write=false",
-    "all:eslint": "dev/foreach-ts-project.sh yarn -s run eslint --quiet",
+    "all:eslint": "DOCSITE_LIST=\"$(./dev/docsite.sh -config doc/docsite.json ls)\" dev/foreach-ts-project.sh yarn -s run eslint --quiet",
     "all:stylelint": "yarn --cwd client/web run stylelint && yarn --cwd client/shared run stylelint && yarn --cwd client/branded run stylelint && yarn --cwd client/browser run stylelint",
     "all:tsgql": "yarn --cwd client/web run tsgql validate -p . --exitOnWarn && yarn --cwd client/shared run tsgql validate -p . --exitOnWarn && yarn --cwd client/browser run tsgql validate -p . --exitOnWarn",
     "build-ts": "tsc -b tsconfig.all.json",
@@ -95,6 +95,7 @@
     "@slack/web-api": "^5.10.0",
     "@sourcegraph/babel-plugin-transform-react-hot-loader-wrapper": "^1.0.0",
     "@sourcegraph/eslint-config": "^0.20.19",
+    "@sourcegraph/eslint-plugin-sourcegraph": "link:client/packages/@sourcegraph/eslint-plugin-sourcegraph",
     "@sourcegraph/prettierrc": "^3.0.3",
     "@sourcegraph/stylelint-config": "^1.1.9",
     "@sourcegraph/tsconfig": "^4.0.1",

yarn.lock

@@ -2680,6 +2680,10 @@
     eslint-plugin-rxjs "^2.1.5"
     eslint-plugin-unicorn "^21.0.0"
 
+"@sourcegraph/eslint-plugin-sourcegraph@link:client/packages/@sourcegraph/eslint-plugin-sourcegraph":
+  version "0.0.0"
+  uid ""
+
 "@sourcegraph/event-positions@^1.0.4":
   version "1.0.4"
   resolved "https://registry.npmjs.org/@sourcegraph/event-positions/-/event-positions-1.0.4.tgz#031541374b725cfb0cbefaa07add020b1a119a60"
@@ -18779,6 +18783,11 @@ require-uncached@^1.0.2:
     caller-path "^0.1.0"
     resolve-from "^1.0.0"
 
+requireindex@~1.1.0:
+  version "1.1.0"
+  resolved "https://registry.npmjs.org/requireindex/-/requireindex-1.1.0.tgz#e5404b81557ef75db6e49c5a72004893fe03e162"
+  integrity sha1-5UBLgVV+91225JxacgBIk/4D4WI=
+
 requireindex@~1.2.0:
   version "1.2.0"
   resolved "https://registry.npmjs.org/requireindex/-/requireindex-1.2.0.tgz#3463cdb22ee151902635aa6c9535d4de9c2ef1ef"