Feedback: Yarn

[arcanis]

Really nice work! I threw together a quick demo of trying to run an unmodified build of Yarn within Nodebox; it almost works, which would be incredibly useful for writing tutorials for our website! Here are some of my findings:

blockers

• The http2 module seems missing. It's used by got, one of our dependencies, so we can't disable it.

• The Brotli functions (in particular zlib.brotliDecompressSync) don't seem supported.

• Bug: new URL('npm:1.2.3').pathname === '//1.2.3'; the // is extraneous and breaks Yarn

• Missing worker support (we have a build that can disable it; I need to try without).

• Bug: The Worker constructor doesn't support the eval parameter.

workarounds

• The trailing \n added to strings going through console.log(...) seems missing from the shell.stdout stream - I suspect it's because you also forward it to the caller's console.log function.

• Speaking of which, it'd be good to have a way to disable this automatic forwarding - either as an option, or possibly when detecting that there are listeners bound to the data event?

• Data written on stdout via process.stdout.write (rather than console.log) don't seem to trigger the shell.stdout callback. I workaround this by doing a process.stdout.write = console.log right before spawning the process.

• I didn't manage to catch the output from shell.stderr - it seems the callback is never called. Yarn doesn't use stderr so it's not a problem for my use case.

• It doesn't seem possible at the moment to set environment variables in the shell before spawning the process. It'd be nice if it was natively supported (for example by an env option on both emulator.shell.create() and shell.runCommand()).

• It seems that even once the process within a shell exits, shell.kill() must be called if we want to run a new command (seems it's just a matter of shell.id not being reset on exit).

nits

• It'd be nice to have an helper function doing the equivalent of new Promise((resolve) => shell.on(exit, resolve));

• The iframe parameter from the Nodebox constructor should probably be moved to connect, since it's not needed until this point. This way the instance can be created before we have the ref to the iframe.

[DeMoorJasper]

Thanks for testing nodebox and providing such a detailed issue/beautiful demo, this is incredibly valuable. I'll work my way through this list later today and hopefully have everything fixed by tonight, will keep you updated.

Update: the issues with shell.stderr/shell.stdout have been fixed, now all console/process output should be forwarded correctly, tested it on your demo and that part seems fixed.

[DeMoorJasper]

A small end of day update here, we now support brotli, implemented the skeleton of http/2 (will implement more of it next week), got past all the errors described in this issue and now seems like it needs worker_threads which was already planned for implementation next week to support nuxt

Regarding environment variables, you can define those when calling runCommand: https://github.com/codesandbox/nodebox-runtime/blob/main/packages/nodebox/api.md#shellruncommandbinary-args-options

Hoping to get this working next week, really excited to see your nice demo work perfectly

[merceyz]

It'd be nice to have an helper function doing the equivalent of new Promise((resolve) => shell.on(exit, resolve));

Wouldn't events.once cover that? https://nodejs.org/dist/latest-v19.x/docs/api/events.html#eventsonceemitter-name-options

[arcanis]

Thanks for the speedy improvements, Jasper!

Wouldn't events.once cover that?

I meant that "wait for a process to exit before spawning another" is a common enough need (especially given that shells don't currently support more than one process) that it might be worth a dedicated function to avoid having to manually bind the events (which works, but is more error prone and perhaps a little harder to discover).

[arcanis]

Quick note: I noticed there's been a regression that makes new URL('npm:1.2.3').pathname === "//1.2.3", instead of "1.2.3": https://codesandbox.io/p/sandbox/vanilla-nodebox-forked-uhqmt1

[DeMoorJasper]

Thanks for reporting that regression, just merged a fix for it, should be on production in a couple minutes

[arcanis]

Indeed! And I see you've started to implement worker support, very nice 👍

I think we're now hitting the problem in https://github.com/codesandbox/nodebox-runtime/issues/22#issuecomment-1457951172: eval: true as worker parameter is ignored, so the worker is invalid and the install hangs (it also seems like an invalid path / file doesn't throw an exception, hiding the issue).

[arcanis]

I got Yarn working, but the following warning messes with the output:

"OutgoingMessage#setTimeout" is not yet implemented. Please file an issue on GitHub.