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

Respect NODE_PATH when resolving modules #3701

Closed
wants to merge 10 commits into from

Conversation

@purkhusid
Copy link
Contributor

commented Jul 24, 2019

This PR allows the usage of NODE_PATH(https://nodejs.org/api/modules.html#modules_loading_from_the_global_folders) in the bsb module resolution.
The exact usecase for me is to enable the usage of bsb within the Bazel NodeJS rules.
The node_modules folder is located in a non-standard directory and this allows me to add that directory to the bsb module resolution.

purkhusid added some commits Jul 24, 2019

@purkhusid

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Not sure why the new test case if failing. I've got it working locally with:

./scripts/ninja.js config                             
./scripts/ninja.js build
node scripts/ciTest.js -bsb -install-global
@bobzhang

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

This seems nice to have, will it replace the support of npm_config_prefix?
I run the command node scripts/ciTest.js -bsb -install-global, I got the same error as CI
EDIT: after doing some research, it seems we should remove the support of npm_config_preix but support NODE_PATH faithfully

@purkhusid

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

@bobzhang I'll remove the npm_config_prefix code then.
Also, I can reproduce the CI error locally but I can also make my changes work locally. Is there any trick to make sure that the new test case is using the updated bsb when doing bsb -make-world ?

I can make this work locally with these commands(I'm using a Mac):

  1. Fresh clone
  2. git submodule update --init && node scripts/buildocaml.js
  3. ./scripts/ninja.js config && ./scripts/ninja.js build
  4. sudo npm i -g --unsafe-perm .
  5. cd jscomp/build_tests/bs_dependencies_node_path_override
  6. NODE_PATH=<path to bucklescript repo>/jscomp/build_tests/bs_dependencies_node_path_override/overridden_node_modules bsb -make-world

There might be some issues with how I set up the new test case but I can't seem to find the issue. Any ideas?

@bobzhang

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

@purkhusid you can ping me on discord. step 6 can be replaced by node input.js

let node_path =
match Sys.getenv "NODE_PATH" with
| node_path ->
Str.split (Str.regexp ";") node_path

This comment has been minimized.

Copy link
@bobzhang

bobzhang Jul 26, 2019

Member

let's stick to Ext_string instead.
The delimiter seems to be platform specific, ';' on *nix, something different on windows

match List.find (fun dir -> Sys.file_exists (dir // Bsb_pkg_types.to_string pkg)) node_path with
| resolved_dir -> resolved_dir // Bsb_pkg_types.to_string pkg
| exception Not_found ->
Bsb_exception.package_not_found ~pkg ~json:None

This comment has been minimized.

Copy link
@bobzhang

bobzhang Jul 26, 2019

Member

checkout Ext_list.find_opt

purkhusid added some commits Jul 26, 2019


let node_path_delimiter =
if Sys.os_type = "Win32" then
';'

This comment has been minimized.

Copy link
@bobzhang

bobzhang Jul 27, 2019

Member

There is Sys.win32

@bobzhang
Copy link
Member

left a comment

I see a CI failure, do you have such failure locally

purkhusid added some commits Jul 27, 2019

@purkhusid

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

@bobzhang Yeah, it's still failing in CI but works locally. but it seems like it's not using the bsb that has the changes that I made. Do I have to do something so that the build_tests compile and use the bsb with my changes?

@bobzhang

This comment has been minimized.

Copy link
Member

commented Jul 28, 2019

@bobzhang Yeah, it's still failing in CI but works locally. but it seems like it's not using the bsb that has the changes that I made. Do I have to do something so that the build_tests compile and use the bsb with my changes?

It seems you forgot to check in changes to bsb.ml

On branch node-path
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)
  (commit or discard the untracked or modified content in submodules)

        modified:   ../lib/4.02.3/bsb.ml
        modified:   ../lib/4.02.3/unstable/bsb_native.ml

purkhusid added some commits Jul 28, 2019

@purkhusid purkhusid closed this Jul 28, 2019

@purkhusid purkhusid reopened this Jul 28, 2019

@bobzhang

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

it seems removing npm_config_prefix triggered some unexpected effect, will fix it

@bobzhang

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

merged in master via #3708 , thanks

@bobzhang bobzhang closed this Jul 29, 2019

@purkhusid

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Awesome, thanks!

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