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

feat(node/dgram): support dgram (udp) node module compat #2205

Merged
merged 12 commits into from
May 17, 2022

Conversation

cmorten
Copy link
Contributor

@cmorten cmorten commented May 9, 2022

Works towards denoland/deno#18324

  • There are a number of APIs that this PR doesn't implement, for the most part because not sure we can right now?
  • Some implemented APIs may differ subtly from Node (inevitable perhaps when trying to match JS / Deno interfaces with libuv! 😅).

@cmorten cmorten marked this pull request as ready for review May 11, 2022 22:16
@bartlomieju
Copy link
Member

@cjihrig can you take a look too?

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Awesome work, lots of tests enabled! This looks mostly good to me, but I'll wait for review from @cjihrig before landing

@@ -38,11 +38,11 @@ export class HandleWrap extends AsyncWrap {
}

ref() {
notImplemented("HandleWrap.prototype.ref");
unreachable();
Copy link
Member

Choose a reason for hiding this comment

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

Why are these unreachable?

Copy link
Contributor Author

@cmorten cmorten May 12, 2022

Choose a reason for hiding this comment

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

As far as can see currently these methods exist purely for defining an interface on classes rather than something that will ever be implemented in this compat layer - felt more accurate to fail with unreachable than not implemented.

Wonder whether they should be defined at all… or if we want them, make use of the inheritance better and not override in all of the other wraps. E.g.

// handle_wrap.ts

class HandleWrap {
  handle!: Handle;

  // ... other code

  ref(): void {
    this.handle?.ref()
  }

  unref(): void {
    this.handle?.unref()
  }
}

// tcp_wrap.ts, pipe_wrap.ts, udp_wrap.ts etc.

class X extends HandleWrap {
  methodA() {
    // ...some code
    this.handle = listener;
  }

  // no need to override `ref()` and `unref()`
}

Or perhaps could just exist as:

ref!: () => void;
unref!: () => void;

If we keep with the overrides elsewhere handling these methods.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work!

@kt3k
Copy link
Member

kt3k commented May 17, 2022

@cjihrig Thanks for your review!

@kt3k kt3k merged commit 0fdb07f into denoland:main May 17, 2022
@cmorten cmorten deleted the feat/node-dgram-compat branch May 17, 2022 07:30
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.

None yet

4 participants