oven-sh / bun

Incredibly fast JavaScript runtime, bundler, test runner, and package manager – all in one
https://bun.sh
Other
74.24k stars 2.77k forks source link

worker_threads.Worker missing `argv` and `execArgv` option #4130

Closed paperdave closed 10 months ago

paperdave commented 1 year ago

https://nodejs.org/api/worker_threads.html#new-workerfilename-options

argv modifies the inner process.argv. we can copy how env is handled but slightly different semantics. should be an easy issue.

execArgv has more semantics in node.js, since those options can control the runtime. but we dont implement these flags. what we will do for this issue is simply make it visible in process.execArgv, so basically the same as process.argv but i think this is an override instead of an append.

these two keys can be added to the global worker, and then the warnings from src/js/node/worker_threads.ts can be removed.

umrkhn commented 1 year ago

Hi @paperdave, Should I start working on this issue, or has it been resolved?

paperdave commented 1 year ago

it has not been resolved. feel free to go for it.

sp90 commented 1 year ago

@paperdave Just recently started learning zig, but i wanna go for it.

Bun is a huge project could you point me in a direction of where this would be best implemented?

umrkhn commented 1 year ago

@sp90 already working on this

sp90 commented 1 year ago

@umrkhn if you go for it may i request to support resourceLimits as well. It's those 3 options on worker_threads im missing

umrkhn commented 1 year ago

@sp90 i don't know on how to implement resourceLimits, also you can open up a new issue to discuss whether it should be implemented or not or maybe @paperdave can comment on this.

umrkhn commented 1 year ago

also i was unable to fix this issue so you can take over from here if you want to

birkskyum commented 1 year ago

@sp90 , will you give it a go?

sp90 commented 1 year ago

@birkskyum i finally got bun dev env running but have minimal experience using zig and have never worked with C nor C++ so i feel blind going in, do i wanna try yes - can you expect it to be done any time soon, no promises :)

So if you are anyone else have a fix, just throw it on here i'll read the code to get a better understanding

birkskyum commented 1 year ago

@paperdave , this is labelled as "Good first issue", but it's blocking a significant number of tickets, and has already proven to be hard for first-time contributors to resolve - so if you consider it trivial, I think it would be worthwhile to just get it out of the way.

bkniffler commented 11 months ago

Agree, @paperdave and @birkskyum, also tried to look into it, but unable to go anywhere. Its the one thing holding us up using bun across all our system, since it makes cloudflare wrangler fail, e.g. when trying to run tests

const { unstable_dev } = require("wrangler");

describe("Worker", () => {
  let worker;

  beforeAll(async () => {
    // error: [bun] Warning: worker_threads.Worker option "execArgv" is not implemented.
    worker = await unstable_dev("src/index.js", {
      experimental: { disableExperimentalWarning: true },
    });
  });

  afterAll(async () => {
    await worker.stop();
  });

  it("should return Hello World", async () => {
    const resp = await worker.fetch();
    const text = await resp.text();
    expect(text).toMatchInlineSnapshot(`"Hello World!"`);
  });
});
muuvmuuv commented 10 months ago

Adding a case: compiling Angular with the new application builder enabled (esbuild/Vite) requires these as well

birkskyum commented 10 months ago

@muuvmuuv , interesting. I believe Analog support is stuck in the same place

muuvmuuv commented 10 months ago

Yes, you are right, Analag also uses this builder under the hood with Vite so I guess this applies to all frameworks with builders that use Vite?

birkskyum commented 10 months ago

@muuvmuuv , pretty much yet - I look very much forward to this one, because even though it's not the biggest implementation effort, it touches a wide variety of the FE ecosystem.

muuvmuuv commented 10 months ago

Is one already actively working on this, or a PR that needs review? If it isn't that much of an effort, I wonder how we can push this further to make Bun “useable” to more people, and so on, let more people consider Bun as a Node/npm replacement in their pipeline.

birkskyum commented 10 months ago

@muuvmuuv , I've not seen any indication of work on this since the "good first issue" label triggered some attempts that unfortunately didn't make it all the way.

muuvmuuv commented 10 months ago

I found the relevant API file: https://github.com/oven-sh/bun/blob/main/src/js/node/worker_threads.ts#L195

Looking at the worker_threads there is a lot more to-do I think. Most of this stuff is used by Vite to enable parallel processing. But argv is a good start.

I am still looking for the relevant Bun/Zig part that adapts the Node Worker API but could only find this cpp file: https://github.com/oven-sh/bun/blob/main/src/bun.js/bindings/webcore/JSWorker.cpp

muuvmuuv commented 10 months ago

Ok, found this web_worker.zig file but can't tell if it's the relevant file: https://github.com/oven-sh/bun/blob/main/src/bun.js/web_worker.zig

Just some hints for someone who wants to work on it, I never programmed in Zig or have exp in building Bun apis. But would love to :D

birkskyum commented 10 months ago

@muuvmuuv , there is a contribution guide here that helped me build it locally - https://bun.sh/docs/project/contributing

sp90 commented 10 months ago

Big thanks to @otgerrogla !! 🎉