Update config docs to warn about antipattern

[callumlocke]

Problem

This is a common pattern:

[options]
module.system.node.resolve_dirname=node_modules
module.system.node.resolve_dirname=src

It lets you write, eg, import foo from 'common/foo' to get hold of src/common/foo.js from anywhere in your codebase, even from a deeply nested module, so you can avoid long ../../../ relative paths.

The problem is, Flow v0.57.0 added a new feature:

• Flow will now only check the files in node_modules/ which are direct or transitive dependencies of the non-node_modules code.

This is a good feature. But for anyone using the above technique, it breaks Flow in a very confusing way: it treats src as a node_modules-like directory, and stops checking all your files in there reliably. It may report errors in some files, but misses many others. Then while you're developing, the watch-driven Flow server will occasionally discover more errors, which mysteriously disappear next time you do a full flow check. It feels indeterminate and unpredictable, and in my case it led to days of debugging.

From the number of issues relating to this, it seems it's easy to miss the problem when you first upgrade past 0.57.0 – the weird behaviour often surfaces much later, because everything seems to be working fine at first (No errors!).

Solution

Update the docs to explain that it is now an antipattern to use module.system.node.resolve_dirname for internal code, and that you should use module.name_mapper instead.

---

Closes #5180
Closes #5316
Relates to #5647
Also closes flow/flow-bin#91

(There have been many more issues stemming from this same problem, but most have been closed as duplicates.)

[STRML]

This one drove us nuts for a long time. Unsure if breaking resolve_dirname was intended or not, but we had to move everything to module.name_mapper to compensate for it (which is far more verbose).

[callumlocke]

we had to move everything to module.name_mapper to compensate for it (which is far more verbose).

Yeah it's a shame it's more verbose. There's also a trick where you can use a dot-slash in resolve_dirname (i.e. ./src/ instead of src), and this seems to opt out of the new 0.57.0 behaviour. But it seems to be an undocumented hack, and it might stop working in future, so using multiple name_mapper lines seems safest. If I'm wrong on this, and the ./ is in fact a supported opt-out, please can a Flow team member let me know and I will update this PR.

[TrySound]

@STRML This was done because everybody asked to not check whole node_modules, but only direct and transitive dependencies.

I think both alias and root resolve are bad practices because every tool can support this features differently or not support at all. This features do not have a spec and they are not even a convention. I usually force to not create deep folders. Also there's solution via monorepos.

[callumlocke]

I think both alias and root resolve are bad practices because every tool can support this features differently or not support at all.

I agree they're not good practices. I prefer to use ../../../ and accept the fact they are ugly, because at least they work automatically with every tool. But in reality many projects use either resolve_dirname or name_mapper for prettier paths. And using one of the two methods (resolve_dirname) is really bad since Flow 0.57.0, so the docs need to warn against it.

The docs update in this PR doesn't recommend magic paths, it just says you should use name_mapper if you're going to do it.

[facebook-github-bot]

@mrkev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[mrkev]

I think this'd be good, but the import is failing. Could you rebase so I can give it a shot?

[mrkev]

bump @callumlocke