Fix no-extraneous-dependencies: Take 2

[cartogram]

Description

Fixes #1001
Follow up from #1026

This PR is split into 2 commits. The first contains only the changes required to make the import/no-extraneous-dependencies rule work for this repo, the second is all the lint fixes required to pass CI.

The fix here was to use the import/resolver setting to override the typescript resolver from plugin:shopify/typescript. The typescript resolver was problematic because it resolves modules to their source location, causing the rule think of them as "internal" (not extraneous) modules. Creating a new configuration and extending it before extending the plugin:shopify/typescript tells the rule to use the node resolver, which looks for these packages in node_modules instead and properly sees them as "external" to the eslint rule.

The downside to having this rule enabled is that it (correctly) enforces all our @shopify/* dependencies to be listed when they are used and thus increases the dependency web and number of "patch" dependency bumps needed (releasing a new version of dependency x causes all related dependencies that use x to need a new version). The largest offender of this was @shopify/react-testing, which is used in all of the @shopify/react-* dependencies.

The fix for this was to add an override for test files that disables this rule. We don't care if @shopify/react-testing is listed or not since its really just needed for development and will never cause a missing dependency error when installed in a consuming package, which is the only thing we really care about here.

[cartogram]

This library uses the Context from koa and would require the consumer to have koa installed in their project. There were a number of packages that met this criteria, and I use the following method to resolve them: 1) Add the packages the host is required to have in the peerDependencies with a pretty loose version range, and 2) add the types packages used to the dependencies.

Why? I added the peerDependency because it is needed (yet unused) in this library. You can't really use a koa middleware as intended without koa, but you also don't need koa to write them. They are, more or less, just functions. I added a loose version range to reduce the chance 2 versions of the package will be installed, in this case koa, as a result of this library.

I added the types in dependencies because there is a middleware, persistedOperationAssociationMiddleware, that uses the Context type in one of its arguments and that function is being exported. Even though the use of the type is tangental, we are not exporting the type itself or expecting consumers will use it, they will use the function that uses the type. For this reason, it needed to be in dependencies.

[lemonmade]

Good rationale. Is this version range too restrictive, though?

[cartogram]

With the resolver changes and re-classification of these libraries (from "internal" to "external"), they are being rearranged as a result of import/order.

[lemonmade]

It is still not clear to me why they should have changed position relative to @shopify/dates?

[lemonmade]

Good rationale. Is this version range too restrictive, though?

[lemonmade]

Hmm, I'm not sure if these are strictly dependencies. Aren't they just used for types? Babel should generally be a peer/ optional dependency IMO

[jgodson]

Thanks for the FYI and the thorough explanation. It makes sense to me 👍. I just had one minor comment/nitpick.

[BPScott]

I wonder if there's some upstream PR we can make to https://github.com/alexgorbatchev/eslint-import-resolver-typescript to allow support for our monorepo use case

[BPScott]

I'm surprised this works - rules later in the config overrode earlier ones - it's how you can enable a rule in a config, then disable it in your local eslint config. I would have thought settings would work in the same way.

[lemonmade]

I think the import plugin iterates over the setting object, which gets merged together by ESLint's config. However, it merges in the order of plugin addition, and then iterates over the object (iteration happens in insertion order), so resolvers added by earlier plugins are actually handled first.

[lemonmade]

Looking good, still a few more questions 👍

[lemonmade]

Is this actually used for it directly?

[cartogram]

It isn't no. It is just imported.

[cartogram]

Might make more sense as a peer dependency.

[lemonmade]

What is this used for?

[cartogram]

It is used for checking if there is a Gemfile, when getting the manifest file. See pathExistsSync below.

function getManifestPath(root: string) {
  const gemFileExists = pathExistsSync(join(root, 'Gemfile'));
  if (!gemFileExists) {
    return;
  }

  // eslint-disable-next-line no-process-env
  return process.env.NODE_ENV === 'development'
    ? `tmp/sewing-kit/sewing-kit-manifest.json`
    : `public/bundles/sewing-kit-manifest.json`;
}

[BPScott]

Looks like it's an alias for the built in fs.existsSync.

https://github.com/jprichardson/node-fs-extra/blob/master/lib/path-exists/index.js#L11

[lemonmade]

Weird spacing

[lemonmade]

whoops

[lemonmade]

Probably want a much less restrictive version here

[lemonmade]

I actually had a bit of a change of heart on this, while technically they do make sense as dependencies, in practice they probably don't because you never consume the Babel dependency directly (only Babel consumes it). So, IMO, dev dependencies

[lemonmade]

It is still not clear to me why they should have changed position relative to @shopify/dates?

[lemonmade]

Neither of the above are dependencies, both can be in devDependencies