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

worker_threads.Worker missing argv and execArgv option #4130

Closed
paperdave opened this issue Aug 12, 2023 · 21 comments · Fixed by #7962
Closed

worker_threads.Worker missing argv and execArgv option #4130

paperdave opened this issue Aug 12, 2023 · 21 comments · Fixed by #7962
Labels
good first issue Something that would be good for new contributors node.js Compatibility with Node.js APIs

Comments

@paperdave
Copy link
Collaborator

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
Copy link

umrkhn commented Sep 10, 2023

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

@paperdave
Copy link
Collaborator Author

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

@sp90
Copy link

sp90 commented Sep 11, 2023

@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
Copy link

umrkhn commented Sep 11, 2023

@sp90 already working on this

@sp90
Copy link

sp90 commented Sep 11, 2023

@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
Copy link

umrkhn commented Sep 12, 2023

@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
Copy link

umrkhn commented Sep 12, 2023

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

@birkskyum
Copy link
Collaborator

@sp90 , will you give it a go?

@sp90
Copy link

sp90 commented Sep 21, 2023

@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
Copy link
Collaborator

@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
Copy link

bkniffler commented Dec 14, 2023

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
Copy link

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

@birkskyum
Copy link
Collaborator

birkskyum commented Dec 22, 2023

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

@muuvmuuv
Copy link

muuvmuuv commented Dec 22, 2023

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
Copy link
Collaborator

@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
Copy link

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
Copy link
Collaborator

@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
Copy link

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
Copy link

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
Copy link
Collaborator

birkskyum commented Dec 22, 2023

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

@sp90
Copy link

sp90 commented Jan 4, 2024

Big thanks to @otgerrogla !! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Something that would be good for new contributors node.js Compatibility with Node.js APIs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants