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

feat(resolver): Add NODE_PATH support #14089

Merged
merged 2 commits into from
Jan 28, 2025
Merged

Conversation

AielloChan
Copy link
Contributor

What does this PR do?

Add resolve check for NODE_PATH.

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

I wrote automated tests

  • I included a test for the new code, or existing tests cover it

  • I ran my tests locally and they pass (bun-debug test test-file-name.test)

  • I checked the lifetime of memory allocated to verify it's (1) freed and (2) only freed when it should be

  • I included a test for the new code, or an existing test covers it

  • JSValue used outside outside of the stack is either wrapped in a JSC.Strong or is JSValueProtect'ed

  • I wrote TypeScript/JavaScript tests and they pass locally (bun-debug test test-file-name.test)

@AielloChan
Copy link
Contributor Author

#13125

Copy link
Member

@paperclover paperclover left a comment

Choose a reason for hiding this comment

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

tiny nitpick but this looks incredibly reasonable

can you add a test that checks that more than one NODE_PATH entries work?

src/resolver/resolver.zig Outdated Show resolved Hide resolved
@AielloChan AielloChan force-pushed the main branch 2 times, most recently from 26d5571 to 8e1f109 Compare September 24, 2024 03:11
@paperclover paperclover self-assigned this Sep 24, 2024
@AielloChan
Copy link
Contributor Author

@paperdave Looks some github action configure was broken. Runner image is Ubuntu, but script try to call a zig.exe

Arc 2024-09-25 11 24 51
/usr/bin/bash: line 1: vendor/zig/zig.exe: No such file or directory
error: script "zig" exited with code 127
error: script "zig:fmt" exited with code 127
error: script "fmt:zig" exited with code [12](https://github.com/oven-sh/bun/actions/runs/11006290644/job/30561806366?pr=14089#step:7:13)7
Error: Process completed with exit code 127.

@bingtsingw
Copy link

@paperdave Hi, is this pr ready to be merged?

@bingtsingw
Copy link

Any progress on this?

@paperclover
Copy link
Member

Any progress on this?

im not sure why this didnt get merged before. i rebased it and will see how tests run.

@paperclover
Copy link
Member

paperclover commented Jan 11, 2025

apologies for the delay. while i did rebase it with a linting error, fixing that error and running tests locally does not pass. so this cant merge as-is (and git locally isnt pushing to this branch so the lint is not fixed either, but thats just removing one line)

@paperclover paperclover marked this pull request as draft January 11, 2025 02:23
Copy link
Member

@paperclover paperclover left a comment

Choose a reason for hiding this comment

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

see comment, failing tests

@AielloChan
Copy link
Contributor Author

@paperclover Reedited! All test case passed in my fork.

@AielloChan AielloChan marked this pull request as ready for review January 17, 2025 06:30
@AielloChan
Copy link
Contributor Author

@paperclover Hello, this feature is very important for serverless environments, such as Aliyun serverless, we can use many build-in node_modules in docker image, by specify NODE_PATH to make it work. Many thanks 🙏

@paperclover paperclover merged commit dd93f08 into oven-sh:main Jan 28, 2025
69 checks passed
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.

3 participants