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

fixes for deno compatibility #387

Merged
merged 3 commits into from
Feb 29, 2024
Merged

Conversation

mash-graz
Copy link
Contributor

Just a first draft PR to feed the CI pipeline, because local testing doesn't work on my machine.

Fixes: #385

Warning! -- there are important parts missing/untested!

Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

I'd be happy to land this with the suggested changes.

@@ -87,7 +87,7 @@ export function createSyncFn(workerPath, debug, callbackHandler) {
}
return result;
};
worker.unref();
// worker.unref();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// worker.unref();
if (worker.unref) worker.unref();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes -- that's a much nicer solution! :)

Nevertheless, I think, it's not enough!

The timeouts in the CI pipeline are most likely caused by hanging processes affected by this particular change resp. broken termination of workers. :(

We have to check for active event listeners or timers...

@@ -21,9 +21,12 @@ import {
registerDispose,
registerIncomingHttpHandler,
} from "../io/worker-io.js";
import { validateHeaderName, validateHeaderValue } from "node:http";
// import { validateHeaderName, validateHeaderValue } from "node:http";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// import { validateHeaderName, validateHeaderValue } from "node:http";
import * as http from "node:http";
const { validateHeaderName = () => {}, validateHeaderValue = () => {} } = http;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's also a very nice improvement!

I just committed both changes, but it still needs a lot more of additional testing and rework.

@guybedford guybedford marked this pull request as ready for review February 29, 2024 16:53
@guybedford guybedford merged commit 1420aad into bytecodealliance:main Feb 29, 2024
8 checks passed
@guybedford
Copy link
Collaborator

Thanks for working on this. Let's at least land for now, we can always improve later.

@mash-graz
Copy link
Contributor Author

Thanks for working on this. Let's at least land for now, we can always improve later.

Yes -- that's a good interim solution, because the merged changes seem to do no harm to already existing use cases and test routines.

I just waited, because I could isolate the non-termination issue as another deno flaw (see: denoland/deno#22629) and I'm still searching a working solution to reuse the existing mocha tests also for deno unit checks (That's rather tricky, because it's very hard to create working import_map-redirections in mixed node/deno-workflows). But it's perfectly fine for me to send you these additional improvements in new PRs.

Thanks for all your help! It's really a pleasure to collaborate with you!

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

Successfully merging this pull request may close these issues.

deno compatibility (use of node:process._rawDebug())
2 participants