Skip to content

Respect NODE_PATH when resolving modules#3701

Closed
purkhusid wants to merge 10 commits into
rescript-lang:masterfrom
purkhusid:node-path
Closed

Respect NODE_PATH when resolving modules#3701
purkhusid wants to merge 10 commits into
rescript-lang:masterfrom
purkhusid:node-path

Conversation

@purkhusid

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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

bobzhang commented Jul 25, 2019

Copy link
Copy Markdown
Member

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

purkhusid commented Jul 25, 2019

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Member

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

Comment thread jscomp/bsb/bsb_pkg.ml Outdated
let node_path =
match Sys.getenv "NODE_PATH" with
| node_path ->
Str.split (Str.regexp ";") node_path

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread jscomp/bsb/bsb_pkg.ml
| resolved_dir -> resolved_dir // Bsb_pkg_types.to_string pkg
| exception Not_found ->
Bsb_exception.package_not_found ~pkg ~json:None

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkout Ext_list.find_opt

Comment thread jscomp/bsb/bsb_pkg.ml

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is Sys.win32

@bobzhang bobzhang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@purkhusid

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Member

@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

Daniel Poul Purkhús added 2 commits July 28, 2019 15:13
@purkhusid purkhusid closed this Jul 28, 2019
@purkhusid purkhusid reopened this Jul 28, 2019
@bobzhang

Copy link
Copy Markdown
Member

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

@bobzhang

Copy link
Copy Markdown
Member

merged in master via #3708 , thanks

@bobzhang bobzhang closed this Jul 29, 2019
@purkhusid

Copy link
Copy Markdown
Contributor Author

Awesome, thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants