TSLint: no-implicit-dependencies should be enabled

[alcuadrado]

Some context: Yesterday Web3.js released a broken package. The problem was that one of its modules was doing a require of a dependency of a devDependency. I took some time to see if this could accidentally happen in EthereumJS and couldn't find anything preventing it.

I think we should enable tslint's no-implicit-dependencies rule, that prevents this from happening.

It has a small annoyance though. For it to actually work, it should only consider dependencies and peerDependencies as valid, so anything installed in devDependencies that is used in a test will be flagged as an error if we use the same tslint config for tests.

There are two ways to solve that:

1. Instructing tslint to also consider devDependencies as installed. This is pretty dangerous.
2. Whitelisting some dependencies. Also kinda dangerous, depending on how many they are. For example, if it's only something like chai, it may be ok, as nobody would use chai outside of a test.
3. Adding // ts-lint-disable-next-line no-implicit-dependencies before any conflicting require in the test files.

This is a great first issue, as the change is very small, but it exposes you to the release process of this library. We need to merge it here, release a new ethereumjs-config version, and update it in the other projects.

[holgerd77]

@alcuadrado Hmm, since this didn't happen (as far as I am aware) during releases in the recent past and at the same time has somewhat heavy implications on the practicality of things, I wonder if it is really worth the introduction?

[alcuadrado]

Oh, I don't think there's a big risk of this happening, as dependencies in EthereumJS aren't frequently changed (which is great IMO).

It's a nice-to-have, and a good (but somewhat tedious) task for someone getting into EthereumJS, so I thought about creating this issue.

[evertonfraga]

We're more exposed to this risk on the VM monorepo (related: ethereumjs/ethereumjs-monorepo#730). I'd like to have this set up some point in time.

[ryanio]

@evertonfraga I added it in 780a088

[holgerd77]

So for what solution do we go here regarding the initial thread on what @alcuadrado proposed?

[holgerd77]

(I never completely got the "dangerous" part from 1. and 2. TBH, would be glad if someone could expand on that)

[evertonfraga]

There are some techniques to simplify the overall dependency tree for monorepos.

1. Manual hoisting: when you define sub-package dependencies or devDependencies on the package.json root.
   Main advantage: you define each dependency that's common to two/several sub-packages only once.

An example of this setup is:

// <root>/package.json
{ …
  "devDependencies": {
    "chai": "latest"
  }
}

// packages/account/package.json
{ …
  "devDependencies": {}
}

// packages/account/test/index.js
const chai = require('chai')

Due to how module resolution work in node (recursively up the directory tree), this will run without issues. And if one publishes a library with that setup, no pandas would die. chai won't be included for two reasons: 1) it's a devDependency, and 2) package was never referenced in package.json. OK.

Now, the danger lies if we do that same thing for a proper dependency or peerDependency. Example:

// <root>/package.json
{ …
  "dependencies": {
    "bn.js": "latest"
  }
}

// packages/account/package.json
{ …
  "dependencies": {} 🚫🚫🚫
}

// packages/account/index.js
const BN = require('bn.js')

The code will run, tests will pass. But the bundled package will not contain BN.js. That's critical, yet so easy to slip through.

2. Lerna hoisting: define dependencies or devDependencies on the sub-packages as before, but when you install all sub-packages modules with Lerna (called bootstrap).
   Pro: faster installs, easier to cache the resulting node_modules.

[evertonfraga]

@alcuadrado did I miss anything?

[alcuadrado]

Nop, I've been following this, and I agree with what you said. Using a monorepo increases the chance of using an undeclared dependency, so IMO also this linter rule's importance.