Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting content root causes modules to be not found #8156

Closed
villesau opened this issue Oct 25, 2019 · 8 comments
Closed

Setting content root causes modules to be not found #8156

villesau opened this issue Oct 25, 2019 · 8 comments

Comments

@villesau
Copy link
Contributor

villesau commented Oct 25, 2019

Flow version: 0.110.0 ->

Expected behavior

Flow should understand path patterns like previous versions does.

Actual behavior

Flow says

"Cannot resolve module `js/test2`."

It's quite common pattern to reduce complexity of imports by setting project root as absolute root so that it can be used in imports. In 0.110.0 -> versions Flow fails these cases.

You can repro the issue by cloning the repo, and opening js2/test.jsfile in editor. Interestingly, yarn flow does not give any errors, which is expected behaviour.

This is probably the original message that sparked such settings: #4186 (comment) and this article has made it more popular: https://medium.com/@sherryhsu/how-to-change-relative-paths-to-absolute-paths-for-imports-32ba6cce18a5

E: create-react-app suggest following: https://create-react-app.dev/docs/adding-flow/ module.name_mapper='^\([^\.].*\)$' -> '<PROJECT_ROOT>/src/\1'

@gabelevi
Copy link
Contributor

Thanks a ton for taking the time to put this together!

I took a look at your repro, and I'm seeing the same behavior I noticed before

If I git clone the repro and run flow check I see

Error ------------------------------------------------------------------------------------------------- js2/test.js:3:18

Cannot resolve module `js/test2`.

   3| import test from 'js/test2';
                       ^^^^^^^^^^



Found 1 error

If I run mkdir node_modules, I then see

Found 0 errors

In the IDE, when I open the files I see the same errors


AFAICT, module.system.node.resolve_dirname=. shouldn't work for anyone. The fact that it works here is almost certainly a bug in the module.system.node.resolve_dirname feature. I'm currently not sure why it works.

If you set module.system.node.resolve_dirname=foo, then any directory named foo would be treated as if it were named node_modules.

Setting module.system.node.resolve_dirname=flow-bug for your repository would be closer to what #4186 (comment) originally suggested. It would treat the root of your directory structure as a node_modules directory and allow paths relative to that. It also somehow avoids the error that occurs above when node_modules is deleted.

However, putting your source files inside of a directory which Flow treats as a node_modules directory is probably not ideal. Flow generally considers code within node_modules as third-party code and less interesting than the user's own code. I worry that this hack might trigger that.


So my thoughts are that two things are appropriate here:

  1. Fix module.system.node.resolve_dirname to disable whatever bug was allowing you to use .
  2. Add module.system.node.allow_root_relative=true or something like that to keep your use case supported

@villesau
Copy link
Contributor Author

villesau commented Oct 25, 2019

@gabelevi experimental.disable_live_non_parse_errors=true does not fully fix the situation. The node modules will not be found after setting the flag :/

Also setting the module.system.node.resolve_dirname=flow-bug (or similar in our main repo) didn't help.

@villesau
Copy link
Contributor Author

villesau commented Oct 25, 2019

module.name_mapper='^\([^\.].*\)$' -> '<PROJECT_ROOT>/\1' did the trick for setting absolute paths, found from here: https://create-react-app.dev/docs/adding-flow/

Another issue with this is that Flow can't resolve any node modules even with empty flowconfig. flow check runs without errors, but opened file reports Cannot resolve module 'recompose' even when added & installed "recompose": "0.26.0" in package.json. It's not only recompose but any node module that doesn't have libdef files.

@gabelevi
Copy link
Contributor

@gabelevi experimental.disable_live_non_parse_errors=true does not fully fix the situation. The node modules will not be found after setting the flag :/

That's actually a little reassuring. I was having trouble understanding how live non-parse errors (e.g. live type errors) was interacting with this issue.

@gabelevi
Copy link
Contributor

And yeah, that name mapper solution would turn any import that doesn't start with a . into an absolute path. That sounds like it would break importing anything from node modules.

facebook-github-bot pushed a commit that referenced this issue Oct 30, 2019
Summary:
#8156 made me aware that some users want to be able to import their code using root-relative paths. They were misusing `module.system.node.resolve_dirname`, which worked for them due to a bug, and experiencing odd behaviors. This stack of diffs demonstrates the behavior, fixes the bug (breaking the use case), and adds first-class support for this use case.

This diff adds a test demonstrating the current behavior.

Reviewed By: jbrown215

Differential Revision: D18171800

fbshipit-source-id: a43e8286c78fd2196980c211b38a347450575789
facebook-github-bot pushed a commit that referenced this issue Oct 30, 2019
Summary:
#8156 made me aware that some users want to be able to import their code using root-relative paths. They were misusing `module.system.node.resolve_dirname`, which worked for them due to a bug, and experiencing odd behaviors. This stack of diffs demonstrates the behavior, fixes the bug (breaking the use case), and adds first-class support for this use case.

This diff fixes the bug. We kept track of which directories contain `node_modules` (or one of its aliases), but we didn't keep track of which alias is within the container. This meant that if you configured your `.flowconfig` with

```
module.system.node.resolve_dirname=node_modules
module.system.node.resolve_dirname=.
```

then any directory `path/to/container` that contained `node_modules/` would lead to us thinking `path/to/container/./` contained node modules. This allowed people to use root-relative paths, but also treated all the code as if it were inside `node_modules/`

The fix is to keep track of which aliases we saw within each container.

Reviewed By: jbrown215

Differential Revision: D18172874

fbshipit-source-id: e6851c16b811d1160f65b647d2bb123649c7e5fd
facebook-github-bot pushed a commit that referenced this issue Oct 30, 2019
Summary:
#8156 made me aware that some users want to be able to import their code using root-relative paths. They were misusing `module.system.node.resolve_dirname`, which worked for them due to a bug, and experiencing odd behaviors. This stack of diffs demonstrates the behavior, fixes the bug (breaking the use case), and adds first-class support for this use case.

This diff adds a new `.flowconfig` option which allows you to import modules using a root-relative path.

Reviewed By: jbrown215

Differential Revision: D18190689

fbshipit-source-id: b1a81e39faa414e3208ae4c804cc3f9aeb91cbcf
facebook-github-bot pushed a commit that referenced this issue Oct 30, 2019
Summary:
#8156 made me aware that some users want to be able to import their code using root-relative paths. They were misusing `module.system.node.resolve_dirname`, which worked for them due to a bug, and experiencing odd behaviors. This stack of diffs demonstrates the behavior, fixes the bug (breaking the use case), and adds first-class support for this use case.

This diff makes `module.system.node.resolve_dirname=.` error. You're only supposed to be passing in valid directory names anyway. `.` and `..` are reserved.

Reviewed By: jbrown215

Differential Revision: D18191349

fbshipit-source-id: 72e3efde4d2f832974bd34e2d6a793cebb5fefbb
facebook-github-bot pushed a commit that referenced this issue Oct 30, 2019
Summary:
#8156 made me aware that some users want to be able to import their code using root-relative paths. They were misusing `module.system.node.resolve_dirname`, which worked for them due to a bug, and experiencing odd behaviors. This stack of diffs demonstrates the behavior, fixes the bug (breaking the use case), and adds first-class support for this use case.

This diff adds a test demonstrating the current behavior.

Reviewed By: jbrown215

Differential Revision: D18171800

fbshipit-source-id: d243b0d429798e3c5b57a21834a7e203d71860a1
facebook-github-bot pushed a commit that referenced this issue Oct 30, 2019
Summary:
#8156 made me aware that some users want to be able to import their code using root-relative paths. They were misusing `module.system.node.resolve_dirname`, which worked for them due to a bug, and experiencing odd behaviors. This stack of diffs demonstrates the behavior, fixes the bug (breaking the use case), and adds first-class support for this use case.

This diff fixes the bug. We kept track of which directories contain `node_modules` (or one of its aliases), but we didn't keep track of which alias is within the container. This meant that if you configured your `.flowconfig` with

```
module.system.node.resolve_dirname=node_modules
module.system.node.resolve_dirname=.
```

then any directory `path/to/container` that contained `node_modules/` would lead to us thinking `path/to/container/./` contained node modules. This allowed people to use root-relative paths, but also treated all the code as if it were inside `node_modules/`

The fix is to keep track of which aliases we saw within each container.

Reviewed By: jbrown215

Differential Revision: D18172874

fbshipit-source-id: c76b8e8b653312d56df078fb57ace9ba56bddd57
facebook-github-bot pushed a commit that referenced this issue Oct 30, 2019
Summary:
#8156 made me aware that some users want to be able to import their code using root-relative paths. They were misusing `module.system.node.resolve_dirname`, which worked for them due to a bug, and experiencing odd behaviors. This stack of diffs demonstrates the behavior, fixes the bug (breaking the use case), and adds first-class support for this use case.

This diff adds a new `.flowconfig` option which allows you to import modules using a root-relative path.

Reviewed By: jbrown215

Differential Revision: D18190689

fbshipit-source-id: e61325ee847361a2873327b8302f3662799d6f58
facebook-github-bot pushed a commit that referenced this issue Oct 30, 2019
Summary:
#8156 made me aware that some users want to be able to import their code using root-relative paths. They were misusing `module.system.node.resolve_dirname`, which worked for them due to a bug, and experiencing odd behaviors. This stack of diffs demonstrates the behavior, fixes the bug (breaking the use case), and adds first-class support for this use case.

This diff makes `module.system.node.resolve_dirname=.` error. You're only supposed to be passing in valid directory names anyway. `.` and `..` are reserved.

Reviewed By: jbrown215

Differential Revision: D18191349

fbshipit-source-id: 716bfd9705bd691332cae0d5ef75ba69601e822b
@gabelevi
Copy link
Contributor

gabelevi commented Nov 4, 2019

Ok! This should be done now!

@gabelevi gabelevi closed this as completed Nov 4, 2019
@villesau
Copy link
Contributor Author

villesau commented Nov 4, 2019

Nice, thanks a lot @gabelevi!

@elie222
Copy link

elie222 commented Nov 5, 2019

I imagine this will trip up a bunch of people. What we did to fix it:

module.system.node.allow_root_relative=true
module.system.node.root_relative_dirname=./app

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants