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

Fix bun run folder #15117

Merged
merged 39 commits into from
Jan 18, 2025
Merged

Fix bun run folder #15117

merged 39 commits into from
Jan 18, 2025

Conversation

pfgithub
Copy link
Contributor

@pfgithub pfgithub commented Nov 13, 2024

This may be a breaking change, so it might wait for 1.2. When using bun <script> or bun run <script>, it now always checks in this order:

  1. Is there a script in package.json with the name? Execute it and exit.
  2. Try loading a module with that name. Can it be done? Execute it and exit.
  3. Is there a binary in node_modules/.bin with that name? Execute it and exit.
  4. (bun run only) Is there a binary in $PATH with that name? Execute it and exit.
  5. Error.

Previously, it would try guessing if it looked like a script name and sometimes it would execute before checking scripts in package.json.

Fixes #13819, Fixes #15743, Fixes #16169, Fixes #16482

New features: If there is a script called myscript.js you can now run it with bun myscript just like node allows

TODO:

  • Docs in cli/run about resolution order
    • Update the outdated line "Bun executes the script command in a subshell. It checks for the following shells in order, using the first one it finds: bash, sh, zsh."
      • On windows, it uses bun shell rather than a system shell. This should be mentioned.
    • Add a new section at the bottom for resolution order
  • Remove entries added to launch.json
  • Remove irrelevant js_printer changes
  • Merge main
  • Pass tests:
    • prisma.test.ts
    • import-custom-condition.test.ts
    • if-present.test.ts
    • self-reference.test.ts

Test failures:

  • bundler-plugin.test.ts (?)
  • 08757.test.ts (windows, absolute path override code)
  • if-present.test.ts (file not found -> module not found)
  • bun-run.test.ts
  • wasi.test.js (windows, absolute path override code)

Consider:

  • Make Run.boot skip generating bun:main and run directly instead. bun:main is generated in entry_points.zig.

Performance:
~0.5ms slower :/ even for ./ which is strange

@robobun
Copy link

robobun commented Nov 13, 2024

Updated 9:46 PM PT - Jan 17th, 2025

@pfgithub, your commit d891e5d has passed in #10003! 🎉


🧪   try this PR locally:

bunx bun-pr 15117

@RiskyMH
Copy link
Member

RiskyMH commented Nov 13, 2024

I think you should also update the docs with this new order to be more explicit :) https://bun.sh/docs/cli/run

@pfgithub pfgithub marked this pull request as ready for review November 19, 2024 02:00
@nektro
Copy link
Member

nektro commented Dec 3, 2024

very excited to see this land; of the top ~1000 packages, this affects 402

@Jarred-Sumner
Copy link
Collaborator

is there a chance we could avoid doing this when a file path with an extension is passed? just so we don't read package.json files up to the root unnecessarily

@pfgithub
Copy link
Contributor Author

@Jarred-Sumner I'm not seeing any performance impact from this PR with hyperfine "bun-45cbc92e81391b46a16c241043ea38c26310e931 run ./a.js" "bun-287f1628e78e2ecee0dfd937d78e3fe3e307187c run ./a.js" --shell=none --warmup=50, although bun-1.1.43 is ~0.2ms faster than latest main

@pfgithub pfgithub marked this pull request as draft January 11, 2025 03:08
var list = std.ArrayList(u8).init(stack_fallback.get());
errdefer list.deinit();

std.io.getStdIn().reader().readAllArrayList(&list, 1024 * 1024 * 1024) catch return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's avoid using zig std for this, they use too much unreachable for this to not cause issues for us.

instead, use bun.sys.File

@Jarred-Sumner Jarred-Sumner marked this pull request as ready for review January 18, 2025 06:06
@Jarred-Sumner
Copy link
Collaborator

This isn't really done, but I'm going to merge it.

Please add a follow-up PR that:

  • Doesn't make entry-point-build and entry-point-run observable
  • Avoids the use of std.fs for reading from stdin and instead uses bun.sys. Zig's use of unreachable will cause crashes for Bun.

There are 2 additional system calls to bun run from this PR. I think that's okay given the context.

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