Prevent missing `@shopify/*` packages from going out to releases

cartogram commented 5 years ago

Overview

Packages found in ./packages often import one another. For example, @shopify/react-server-webpack-plugin makes use of @shopify/ast-utilities. In these cases, the imported package must be listed in the importing package's package.json. This can be easy to forget as there are no build, development, or publishing error reported until the package is released and installed in a consuming project. The consuming project will get an error, such as '@shopify/ast-utilities' modules not found.

An easy temporary fix when this happens is to install the missing package in the consuming project, but the permanent fix is to add ’@shopify/ast-utilities' to @shopify/react-server-webpack-plugin package.json#dependencies. This will require a re-release, which can be time-consuming.

Solutions

1. Lint

We can lint for missing packages and fail the lint step. A rule exists called import/no-extraneous-packages from eslint-plugin-import, but this rule is recognizing the @shopify namespace imports as “internal” (not “external”) causing them to be skipped over/ not reported as extraneous.

To see the problem:

Hack the typescript config in node modules and turn on the rule. (I am not sure why we have it turned off at the moment, the comment says it has something to do with not supporting typescript, but I haven't experienced any issues with the rule https://github.com/Shopify/eslint-plugin-shopify/blob/master/lib/config/typescript.js#L41)
Remove the @shopify/ast-utilities package from react-server-webpack-plugin and run yarn lint '*/**/react-server-webpack-plugin/**/**.ts’ and you should see 2 errors. This is wrong because there is no indication of the missing ast-utilities package we just deleted.
Now run $ node --inspect-brk node_modules/eslint/bin/eslint.js '*/**/react-server-webpack-plugin/**/**.ts’ from the quilt repo.
Go to chrome://inspect and adding a breakpoint In on line ~123 of the no-extraneous-import rule.
Change the import type condition to check for both external and internal, you should now see 3 errors instead of 2.

I haven't been able to figure out why this is happening. It could be a configuration problem (I tried a number of different settings) or could require a patch to the repo.

2. Run a script/test

This can be wrapped into a jest test, but something that runs in CI and checks to make sure that any imported packages are also listed in the package.json.

A rough example below.

const path = require('path');
const {readdirSync} = require('fs-extra');
const grep = require('simple-grep');
const uniq = require('lodash/uniq');

const packagesPath = path.resolve(__dirname, '..', 'packages');
const packageNames = readdirSync(packagesPath).filter(
  file => !file.includes('.'),
);

packageNames.forEach(checkDependencies);

function checkDependencies(package) {
  const packageJson = path.resolve(packagesPath, package, 'package.json');
  let pkg;

  try {
    pkg = require(packageJson);
  } catch (error) {
    // swallow error
  }

  if (!pkg) {
    return;
  }

  const {dependencies = {}, devDependencies = {}, peerDependencies = {}} = pkg;
  const allDependencies = [
    ...Object.keys(dependencies),
    ...Object.keys(devDependencies),
    ...Object.keys(peerDependencies),
  ];
  const missingDependencies = [];

  grep(`@shopify/`, path.resolve(packagesPath, package), function(list) {
    list.forEach(item => {
      if (path.extname(item.file) !== '.ts') {
        return;
      }

      item.results.forEach(({line}) => {
        const parts = line.split('from');

        if (parts.length !== 2) {
          return;
        }

        const imported = parts[parts.length - 1]
          .replace(/'/g, '')
          .replace(/;/, '')
          .trim();

        if (!allDependencies.includes(imported)) {
          missingDependencies.push(imported);
        }
      });
    });

    if (missingDependencies.length > 1) {
      console.warn(
        `Missing Dependencies found in ${package}. You must include ${uniq(
          missingDependencies,
        ).join(', ')} in the package.json for this package.`,
      );
    }
  });
}

GoodForOneFare commented 5 years ago

I think this is similar to https://github.com/benmosher/eslint-plugin-import/issues/1174. The final comment in that thread has a workaround that I imagine is similar to the script above.

cartogram commented 5 years ago

@GoodForOneFare Had a look at that issue and did try the script, but had no luck on getting the missing errors using it. That said, I didn't spend a lot of time digging into why it didn't solve the issue and can try something along those lines again.

cartogram commented 5 years ago

@GoodForOneFare minimal example of the missing @shopify dependencies not being reported using the workaround config: https://github.com/Shopify/quilt/compare/no-extraneous-packages?expand=1

That branch should produce 3 errors, but only has 2.

After digging into things, the issue doesn't seem to be with those configs but more so to do with this PR https://github.com/benmosher/eslint-plugin-import/pull/1294. Where packages that begin with @ are identified as internal from this function which then causes the rule to early return here.

cartogram commented 5 years ago

Update: This function grabs the first part of an internal module (in our case it would be @shopify and checks that a folder matching that name is in node_modules.

You can override the external folders with 'import/external-module-folders': ['node_modules', 'packages'], but it is still looking for an @shopify folder and not an ast-utilities folder.

Replacing line 33 return !path || folders.some(folder => -1 < path.indexOf(join(folder, packageName))) with return -1 < path.indexOf((0, _path.join)(folder, packageName)) || -1 < path.indexOf((0, _path.join)(folder, name.split('/')[1])) fixes it.

Still not sure what the best way to implement a fix is.

cartogram commented 5 years ago

Organizing our packages inside of packages/@shopify instead of just packages might actually fix this. Such as in https://github.com/textlint/textlint/tree/master/packages/%40textlint.

Shopify / quilt