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

Fix definition of "listen" of net$Server and its subclasses #8290

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Rob--W
Copy link

@Rob--W Rob--W commented Feb 7, 2020

The listen signature of net$Server did not match the definition of
the actual API at https://nodejs.org/api/net.html#net_server_listen

In particular, flow failed to detect invalid handles and options due to
the use of an object with all-optional properties, and the use of
Function (which is an alias for any).

This patch fixes the following issues:

  • Remove repeated method declarations of net$Server from its subclasses, and add missing listen methods to net$Server.
  • Callbacks are no longer typed Function (which is equivalent to any), but () => mixed.
  • Rename the listen's handle parameter to options.
  • Require the options object to either contain path or port.
  • The listen method with parameter handle has a type definition.

Fixed the following issues with node_tests:

  • All existing node_tests that were marked with "These should fail" are now failing as expected.
  • handle renamed to options.
  • fd renamed to path (string values are path, not fd).
  • Add new passing tests for listen(handle, ...).
  • Treat empty objects as a failure (because port or path is required).

The number of expected node_tests failures increased from 115 to 131.
12 of those are tests that were marked as "failing" but did not.
The other 4 are signatures that were marked as "passing" but should have
failed (fd with string value instead of number, and empty object).

The `listen` signature of `net$Server` did not match the definition of
the actual API at https://nodejs.org/api/net.html#net_server_listen

In particular, flow failed to detect invalid handles and options due to
the use of an object with all-optional properties, and the use of
`Function` (which is an alias for `any`).

This patch fixes the following issues:

- Remove repeated method declarations of `net$Server` from its subclasses,
  and add missing `listen` methods to `net$Server`.

- Callbacks are no longer typed `Function` (which is equivalent to `any`), but `() => mixed`.

- Rename the `listen`'s `handle` parameter to `options`.

- Require the `options` object to either contain `path` or `port`.

- The `listen` method with parameter `handle` has a type definition.

Fixed the following issues with `node_tests`:

- All existing `node_tests` that were marked with "These should fail" are now failing as expected.

- `handle` renamed to `options`.

- `fd` renamed to `path` (string values are `path`, not `fd`).

- Add new passing tests for `listen(handle, ...)`.

- Treat empty objects as a failure (because port or path is required).

The number of expected `node_tests` failures increased from 115 to 131.
12 of those are tests that were marked as "failing" but did not.
The other 4 are signatures that were marked as "passing" but should have
failed (`fd` with string value instead of number, and empty object).
@jamesisaac jamesisaac added the Library definitions Issues or pull requests about core library definitions label Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Library definitions Issues or pull requests about core library definitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants