[FEATURE] Add module_alias resolution capabilities

[bradennapier]

Hey Y'all!

So first of all, never used OCAML in my life so excuse any flaws in style. Mostly just tried to mimic styling I saw done elsewhere as I implemented this feature. I did add tests and it is definitely fixing the issues I have been having thus far!

So there are a ton of people that use things like Webpack, Rollup, or something like babel-plugin-module-resolver (including Flow team recently I noticed ;-)).

The Feature

This adds an option that is very similar to module.system.node.resolve_dirname but with slightly different handling. It is meant to be used in situations where Webpack's resolve.modules or babel-plugin-module-resolver (as well as multiple other bundlers which offer this type of functionality).

It basically allows a separated place to define paths that should be aliased but type-checked normally since its actually part of the main codebase in this case.

Note: aliases are resolved before any resolve_dirname configurations.

[options]
module.system.node.resolve_alias=shared
module.system.node.resolve_alias=src

In the case of something like the image below:

We can then do the following in ComponentOne or ComponentTwo:

/* @flow */
import A from 'A';
import B from 'B';
import C from 'C';

this would then mimic the Webpack configuration of:

module.exports = {
  //...
  resolve: {
    modules: ['shared', path.resolve(__dirname, 'src'), 'node_modules']
  }
};

or the babel-plugin-module-resolver configuration of:

{
  "plugins": [
    ["module-resolver", {
      "root": ["shared", "./src"]
    }]
  ]
}

Order of precedence would be C (shared), B (shared), then A (./src)

The Problem This Fixes

The general idea is to remove the need to use relative paths everywhere and anywhere. At this point I can't even program without it! So essentially people will do a few possible things:

Have a Resolution to their Root

module.system.node.resolve_dirname=./src
module.system.node.resolve_dirname=node_modules

This way they can import from their root as modules (@see Webpack Documentation) (@see babel-plugin-module-resolver Documentation)

Use a customized styling for node_modules

In my case, I generally use a shared folder and set it up to resolve the same as node_modules would. This is especially useful in React applications as it provides the ability for components to easily share pieces of code at any level.

While the resolve_dirname kind of works better here (with the other example it breaks the code completely), the problem is that when you're building a file it won't work until its been imported from outside a shared folder.

https://github.com/bradennapier/flow-modules-bug-repro

This shows the issue when using it this way.

Why resolve_dirname doesn't work for this

The issue with resolve_dirname is that doing this will generally break the entire application due to a change in 0.57.0

New Features:
* Flow will now only check the files in `node_modules/` which are direct or transitive dependencies of the non-`node_modules` code.

This is extended to any defined resolve_dirname folders even though it's largely used for this purpose. We have since discovered that using a relative path can fix it but there are still lingering issues in these cases.

Examples of People having issues that this solves

Just did some quick searches for examples

#5180
flow/flow-for-vscode#289
#4103
#5494
#5819
#5647
#5586
#5385
tleunen/babel-plugin-module-resolver#233
#5778
#5860
facebook/create-react-app#4105
#6966
tleunen/babel-plugin-module-resolver#235

As well as many more if you search through the various flow repos.

Testing on OSX

If you don't want to build the source and run OSX you can use the binary I built. There is a compiled binary for OSX located in the repro i linked within a branch: https://github.com/bradennapier/flow-modules-bug-repro/tree/feature/fixed-by-pr-7185/bin

[ugeng]

Hi @bradennapier! Looks like you've done very useful and highly-demanded task here!
I didn't see your code and have a question regarding of its capabilities in lerna-managed monorepos.
Will it work if I have only one .flowconfig in monorepo's root, where I describe all the aliases (say, all my packages have same structure)?
Thanks!

[bradennapier]

Hmm well theoretically yes it would work fine for a monorepo I do not see why not. You’d have to use a non relative “src” so it would bubble up in each repo to find the right root path for each but no reason it wouldn’t work great to solve that issue.

I’d imagine if not then this at least provides the ground work for what would be needed to support it. I do have a monorepo I’ll test it on at some point when I have time.

In the end this simply provides a mechanism to provide module-style resolved folders that act like normal app source instead of downloaded packages in flows eyes.

With it you should be able to utilize a combination of this and name mapper to achieve most configurations I have seen.

Hopefully the flow team sees this and agrees soon! For now I’m using it and it’s already done wonders for my programs. 😇

[ugeng]

@bradennapier, sounds wonderful! No doubt in that!
I've mentioned that issue because when I use name_mapper like below:

module.name_mapper='^containers' ->'src/containers'

Flow throws famous 'can not resolve module' error in every package.

I definitely sure that you predict and avoid such issues in your PR, but it should be noticed though.

Thank you! Can't wait for PR's being approved.

[bradennapier]

Have you tried the following:

module.name_mapper='^containers\/\(.*\)$' -> '<PROJECT_ROOT>/src/containers/\1'

Not sure if the substitution is at all necessary but its how all mine are done :-P

You could also try

module.system.node.resolve_dirname=./src

which should resolve all directories in src as modules if that is a desired effect. Although I am not positive if this is a fix that was purposefully produced. I saw no evidence in the codebase although I admit my understanding was limited of many of the aspects. It seems it's a result of the relative directories not being resolved when combined with the base directory when checking to see if they are in the given directory.

Try the Binary

If you feel so inclined and use OSX just give it a try and let me know if it works ;-) https://github.com/bradennapier/flow-modules-bug-repro/tree/feature/fixed-by-pr-7185/bin the osx binary is in there.

[ugeng]

hi, @bradennapier! I've just tried a new binary in my lerna-managed monorepo with yarn workspaces enabled.
I've tuned my .flowconfig like following

and checked that i'm on new version (currenly i'm on 0.84 because of react-redux typings issue)

All seems fine.

And it definitely works in my "host" package

As you can see, it provides path to module which, as I can assume, tells us that the module resolved.
But in my "library" packages it works a bit differently.

It does not provide neither module path, nor any module info.. But vscode plugin doesn't complain about it, so it probably works. To ensure that I ran yarn flow check --show-all-errors and it was no errors in console. So i'm not sure that I can call it an issue! 😄

PS. In order to simplify webpack config I use nwb starter tool both in main (hosting) package and in library packages. I don't know how it could impact on situation but it probably could.

PPS. You set version=0.86 but such version already released and have issues with react-redux typings

PPPS. Thank you for the great job you've done. Seems like all almost perfect )

[bradennapier]

Yeah I leave it out of the List which would define the alias as a “module” to Flow so it probably is the reason it won’t show up there as a module.

It’s an alias so it’s simply resolving the given alias in a node_module way while preserving standard flow type checking that is removed for modules.

Thanks for testing! Glad it works for your needs :)

Edit:

Just realized you mentioned it looks different in vscode in the two places but that it shows as module in one. I’ll have to look into that but it’s likely something with the vscode plugin.

[bradennapier]

@jbrown215 any chance i can get a review on this PR? I think its a really solid feature and i miss it already since i updated to latest and havent yet built my version of it!

I updated and fixed conflicts with newest version

[jbrown215]

Hey @bradennapier:

I really appreciate that you took the time to make this PR. Unfortunately, I'm not a good person to review it. I'll import it and see if anyone else can take a look, but I can't guarantee you someone will get to it.

[facebook-github-bot]

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

[bradennapier]

Thanks @jbrown215 ! Looks like build is failing with my conflict resolutions. If someone could review the concept and let me know they'd merge it I will absolutely fix it right away. I probably will anyway but would be nice to have someone with the proper authority or w/e approve the spec.

[jbrown215]

I recommend waiting for someone to respond before you put more work into this. I don't want you to keep putting time into something that we may decide is something we don't want or cannot put in the time to support.

[bradennapier]

Will do, thanks again!

[vjpr]

You could also try module.system.node.resolve_dirname=./src which should resolve all directories in src as modules if that is a desired effect.

FYI: This means modules/foo.js won't work, but modules/foo/index.js will, for import foo from 'modules/foo.

Actually nope. My issue was that my source must be inside the src/modules dir so that it can find src/modules/foo.js.

[vjpr]

@jbrown215 Can you please review this as discussed on Twitter.

[bradennapier]

I would note that this part of the codebase has changed a little bit so would need to adjust accordingly, but happy to look into that should the general concept of the feature be approved for further acceptance (which I would personally like to see).

We do run into it on every project and it's fairly common practice to use module alias for webpack setups.

[nmote]

I chatted briefly with @samwgoldman who had some concerns that this might cause problems with rechecks. I'm still looking into this potential issue and I'll have more to say on Monday. I'm optimistic that we can get this merged in some form in the near future.

[nmote]

I spent some time investigating this today, and I think this should work out. If I'm missing something, hopefully @samwgoldman corrects me.

Basically, the concern was that complicating module resolution in this way could lead us to miss rechecks. Specifically, if foo.js depends on bar.js, but another bar.js elsewhere is added, and due to module resolution rules, foo.js now depends on the new bar.js, we have to make sure to recheck foo.js against the newly created bar.js.

Fortunately, it looks to me like our logic for tracking this should generalize quite easily. In fact, I don't think this PR will need to do anything special for everything to fall into place.

I could be misunderstanding something (this is my first time looking at this logic), but I'm optimistic that this can be merged shortly. @bradennapier, when you get a chance could you rebase this so that I can play around with it and make sure that everything behaves the way I expect?

[bradennapier]

Alright so rebased and cleaned up the commits a bit here, confirmed build worked and the feature still operates as expected.

There are a couple notes to make here on considerations. None of the behaviors discussed deviate from the general behaviors expected, but good notes to consider:

---

Webpack Alias Deviation

In Webpack aliases you can do something like:

resolve: [
   'shared',
   'node_modules',
   'backup_modules'
]

which would then resolve in the order provided if possible. However, in our scenario (which is the only way i have ever even considered using this feature, and the way i believe most people do use it), our custom aliases would also resolve before node_modules so having a folder locally which shadows a node_module would instead resolve our alias.

This is the desired behavior but worth mentioning.

---

Good flow lint situation

While again consistent with the behavior of the features these are based upon, there are some situations that flow could be an even more useful tool than the others with its extra lint capabilities.

This edge case is included in the tests that I pushed as well.

Imagine we have a directory structure like so:

root/
  |-- shared/
      |-- something/
         |-- index.js
  |-- node_modules/
      |-- something/
         |-- index.js
         |-- else.js

and a .flowconfig like this:

[options]
module.system.node.resolve_alias=shared

Now if we import something from any file it will resolve to root/shared/something/index.js. HOWEVER, if we instead import something/else then it would end up resolving from root/node_modules/something/else.js. This is good in many ways but could also lead to unwanted behavior.

In these cases, allowing the user to select cases they'd want to be warned of these cases would be ideal.

1. Warn if a path has a similar resolved path in the alias but ends up resolving in the node_modules (like above)
2. As another option, warn if a similar resolve path is in an alias but it ends up resolving in an alias path further down in the tree. (aka if we have root/shared/something/else and root/src/shared/something it would warn when an alias <--> alias conflict occurs.

[lints]
resolve-conflicts-self=error
resolve-conflicts-bounds=error

honestly not sure best names to give them but essentially one for controlling alias <--> node_modules conflicts and another controlling alias <--> alias conflicts (and potentially node_module <--> node_module conflicts for when the user also defines multiple resolve_dirname values.

This would be tricky if we wanted it to warn if there was a potential conflict further up the tree (we don't, it should have standard shadow behavior), instead we just want to flag if we were able to traverse into a top level of a given value but did not resolve then moved up the tree and did find a value to resolve.

Just a thought!

---

TypeScript Comparison

TypeScript is actually not currently capable of emulating this behavior. It does have the paths option which gets close but it is not the same thing.

When you want to have TypeScript resolve ./src as-if they were modules then it's simple:

{
  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "*": ["./src/*", "./shared/*"]
    }
  }
}

However, in the case you desire the actual Webpack alias behavior you are out of luck as far as I am aware. I have added the ts-comparison folder in my repro to highlight this case (I use jsconfig.json which is what VSCode uses to resolve js files using TypeScript)

Specifically in this file the TypeScript method is not able to figure out this case

Not to mention its hard to beat the simplicity of the more powerful and simple to understand solution this PR provides:

[options]
module.system.node.resolve_alias=shared
module.system.node.resolve_alias=./src

and finally the matching webpack.config.js for this case from the root dir would look something like:

const path = require("path");

module.exports = {
  module: {
    resolve: ["shared", path.resolve(__dirname, "./src"), "node_modules"]
  }
};

[bradennapier]

Failure Case (Non Critical but Important)

@see this commit where the highlighted value should not be an error since are essentially giving it an absolute path (after it is expanded from root) to look at.

So the above example actually led me to finding a failure case in how this should work (and it also fails in the case of using module_dirname which was my suspicion all along. The reason using ./src in module_dirname is a workaround is because of this bug.

Essentially it is not honoring relative paths properly. I also think it should properly handle <PROJECT_ROOT> - a bit beyond me as-to how to do this properly. I assume its just about calling expand_project_root_token_to_string at the right time or doing something similar to module_name_mapper since that does expand the token as-desired, just not familiar enough with OCAML to fix this case (and the actual relative path case ./).

Essentially at this time:

module.system.node.resolve_dirname=./one
module.system.node.resolve_alias=./two

is resolving the same as:

module.system.node.resolve_dirname=one 
module.system.node.resolve_alias=two

and the following does not work at all (but should)

module.system.node.resolve_dirname=<PROJECT_ROOT>/one
module.system.node.resolve_alias=<PROJECT_ROOT>/two

I'm guessing this is just a matter of using this func in one of the handlers using the ~node_modules_containers

[facebook-github-bot]

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

[samwgoldman]

I reviewed the module_js.ml part of this change only. @nmote please review the rest.

My main request is that you add a few tests around the incremental / recheck behavior. You can look at tests/incremental_path/ for an example which exercises the phantom_dependents tracking behavior.

Yesterday I was concerned that the lazy_seq logic was buggy because we would not track the unvisited (due to laziness) paths. Now I think that it's actually OK due to precedence, but it would be nice to have at least one test to guarantee that.

I am especially curious about cases where there are (a) multiple resolver aliases (b) both resolver aliases and resolver dirnames, and those directories appear / disappear in a way that affects what a module reference resolves to.

It would also be nice if the issue with relative paths was addressed. I'm not thrilled with the prospect of merging a feature with known bugs.

As a parting note, we won't use this configuration at FB, which means that automated tests are particularly important to prevent regressions as this logic evolves. The "static" behavior is simple enough, but incremental logic is always complicated enough to warrant testing.

[nmote]

Hey, just to make sure that everyone is on the same page here: I'm planning to review the rest of the PR once @samwgoldman is satisfied with the part that he has started reviewing.

[bradennapier]

I spoke with Sam and the only thing needed is to do the extra tests which I haven’t had time for due to moving across the country. Hope to get to it soon.

[nmote]

Okay, no rush on our end, I just wanted to make sure you weren't waiting for me.

[goodmind]

@bradennapier are you available for this PR?

[davidroeca]

Any updates on this PR? The current work-around is to have a messy flow config with a bunch of module name mappers, which becomes cumbersome.

[nmote]

I haven't heard anything from @bradennapier in some time, so I'm going to close this. Anyone, feel free to ping me on a replacement PR if you are interested in driving this to completion.