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

deno compatibility (use of node:process._rawDebug()) #385

Closed
mash-graz opened this issue Feb 27, 2024 · 10 comments · Fixed by #387
Closed

deno compatibility (use of node:process._rawDebug()) #385

mash-graz opened this issue Feb 27, 2024 · 10 comments · Fixed by #387
Labels
enhancement New feature or request

Comments

@mash-graz
Copy link
Contributor

mash-graz commented Feb 27, 2024

It would be nice, if jco and its transpilation capabilities could be used/executed also by deno and other popular JS runtimes.

Right now we get an error on every execution attempt:

deno run -A npm:@bytecodealliance/jco --help
error: Uncaught SyntaxError: The requested module 'node:process' does not provide an export named '_rawDebug'
    at <anonymous> (file:///home/mash/.cache/deno/npm/registry.npmjs.org/@bytecodealliance/preview2-shim/0.15.1/lib/io/worker-io.js:36:10)
@guybedford
Copy link
Collaborator

Yes we are interested in this, but it's not currently a development priority.

Any PRs to improve Deno compatibility will be gladly merged though.

@mash-graz
Copy link
Contributor Author

The use of _rawDebug() seems to be in place since #334

@guybedford guybedford added the enhancement New feature or request label Feb 27, 2024
@guybedford
Copy link
Collaborator

Note that the main blocking issue for Deno support is related worker threads behaviours I believe.

@mash-graz
Copy link
Contributor Author

mash-graz commented Feb 27, 2024

This particular issue of _rawDebug() should be easily solvable by using a more common logging mechanism, but I just couldn't get further. There may be other much more significant obstacles waiting behind this more or less trivial flaw. But in general it would be very appreciable, if deno compatibility could solved and tested in your CI pipeline.

Jco transpilation seems to be the only option to utilize WASM components on deno deploy. That's why a working execution of this tool on this particular runtime would be really helpful.

@mash-graz
Copy link
Contributor Author

mash-graz commented Feb 27, 2024

On a quick search I found three incompatibilities:

  1. _rawDebug -- can be replaced by console.error
  2. lutimesSync -- that's indeed still missing in deno (Implement missing node:fs APIs denoland/deno#18218) but there are workarounds available in external sources.
  3. worker_threads in deno are missing the unref function. But this shouldn't be a significant obstacle as long as all connected message handlers and timers are removed properly.

Although I could quickly figure out temporal workarounds for all of these conflicts, they still should be patched in more reliable manner by someone more familiar with this code base.

@guybedford
Copy link
Collaborator

guybedford commented Feb 27, 2024

@mash-graz thanks for summarizing your findings here. With those changes were you able to get the implementation to work in Deno?

@mash-graz
Copy link
Contributor Author

mash-graz commented Feb 27, 2024

yes -- after bypassing these three obstacles I could run jco and its transpilation command.

But I did it just as a very quick and incomplete sketch to look how much efforts are needed.
I'm still looking how to realize it in a more adequate fashion.

questions:

  • Does your code really require _rawDebug to analyze any internals of the JS runtimes async executor or similar low level details, or is it just utilized because of performance reasons?

  • fixing the lutimesSync issue is somehow tricky, because deno doesn't provide the necessary APIs. In this case I would simply throw a verbose not implemented error on any attempt to change timestamps of symlinks as long as this function isn't available in a runtime resp node emulation.

    Something like this modification of setTimesAt(...) in preview2-shim/lib/nodejs/filesystem.js:

      if (!pathFlags.symlinkFollow && !lutimesSync){
        throw new Error("Changing the timestamps of symlinks isn't supported");
      }

For the worker_thread.unref call resp. proper termination of the affected subprocesses I still haven't figured out a nice solution. I'm sure, more experienced developers will immediately know, how to solve this kind of challenge in the most efficient manner.

@guybedford
Copy link
Collaborator

If that is the case, then we should update and land #295 with these changes, because I didn't realise we were that close.

  • _rawDebug is important for log timing since it is the only stdout that immediately flushes in Node.js, but could be easily replaced with (_rawDebug || console.error)(msg) for Deno compat. The problem is specifically the import { _rawDebug } syntax, which should be removed and we should just use const rawDebug = process._rawDebug || console.error.bind(console); instead.
  • lutimesSync again this is an import issue - if we do import fs, { otherFns } from 'node:fs' and then we can check for fs.lutimesSync and throw your trap as you have written.
  • For worker_thread.unref again this should be if (worker_thread.unref) worker_thread.unref().

Would you be interested in working on a PR here?

@mash-graz
Copy link
Contributor Author

I'll try to prepare a PR tonight, although I'm sure, others are much faster and writing better code. ;)

Concerning the third point of the list (worker_thread.unref) we'll need most likely more task termination cleanup, because only removing the unref call doesn't work. The process just will not stop anymore in this case. :(

@guybedford
Copy link
Collaborator

That would be amazing if you are able to create a PR - as an experimental target it is okay if it is not correct or doesn't work properly, or stalls on exit to start. The important thing is that the CI still passes for the current Node.js implementation, and we improve this experimental target for those interested to play around with further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants