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

add <PROJECT_ROOT> token support for node.resolve_dirname #7083

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@tanhauhau
Copy link
Contributor

commented Oct 23, 2018

add <PROJECT_ROOT> token support for node.resolve_dirname in .flowconfig

@nmote

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

Overall this looks pretty reasonable. I'm not familiar with this option so I'll have to spend a bit of time reviewing to make sure I'm comfortable with this change.

If you are still interested in contributing, could you rebase and I'll take a closer look?

Also, it looks like the change to trim_lines is extraneous, could you either remove that change or split it out into a separate pull request?

Sorry for the delay here. If you decide to move forward, I'll personally make sure this gets reviewed.

@tanhauhau tanhauhau force-pushed the tanhauhau:tanhauhau/project_root branch from dd42247 to 6216d11 Jan 9, 2019

@tanhauhau

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

hi @nmote i've rebased my changes. 😄

@nmote

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

@tanhauhau I just took a closer look and it looks like this option is already resolved relative to the project root. I put this line in my .flowconfig:

module.system.node.resolve_dirname=moduless

A module I placed in the moduless (intentionally misspelled just to make sure it was really due to this setting) directory was resolved as expected.

What is the motivation for this change?

@tanhauhau

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

My main motivation is for my lerna based monorepo project setup:

packages:

  • pkgA
    • dependencies:
      • pkgB
      • ext_A
  • pkgB
    • peerDependecies:
      • ext_A
    • dependencies:
      • ext_B

dependencies within the project are symlinked, so you get:

  • pkgA
    • node_modules/
      • pkgB (symlink)
        • node_modules/
          • ext_B
        • src/
          • code_B
      • ext_A
    • src/
      • code_A
  • pkgB (actual code)
    • node_modules/
      • ext_B
    • src/
      • code_B

in code_B, I imported ext_A, (since it's a peer dependency). when I run flow in pkgA, it throws error because it couldn't resolve ext_A, because pkgB is resolved to /pkgB (the actual location) instead of /pkgA/node_modules/pkgB, and then resolve_dirname will recursively look upwards, and couldn't find ext_A in the /node_modules/.

So, I wish to add <PROJECT_ROOT> token, which is available to other options, to resolve from the absolute path of ext_A/node_modules.

Hope it explains.

@nmote

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

My point though is that Flow, today, appears to resolve these paths relative to the project root (that is, the location of the .flowconfig) It appears that setting the option to ext_A/node_modules today would be equivalent to setting the option to <PROJECT_ROOT>/ext_A/node_modules after your PR. Am I missing something?

@tanhauhau

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

for my case, my .flowconfig resides in pkgA/

@nmote

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

So are you thinking you would use <PROJECT_ROOT>/../pkgB/node_modules? Does simply writing ../pkgB/node_modules not have that effect today? <PROJECT_ROOT> will resolve to the location of the .flowconfig, which in your case would be pkgA/.

@tanhauhau

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

@nmote

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

Okay, I'll try it out myself as well. Let me know what you find.

@nmote

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

I checked and Flow today does not support that usecase. Could you test your PR and confirm that it it allows you to use <PROJECT_ROOT> to traverse outside of the root directory?

@nmote

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

@tanhauhau, I'm closing for now since I haven't heard whether this PR addresses your problem. Feel free to reopen it if you would like to move forward

@nmote nmote closed this Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.