Skip to content

Conversation

@lpinca
Copy link
Member

@lpinca lpinca commented Feb 22, 2020

Make virtual methods throw an ERR_METHOD_NOT_IMPLEMENTED error instead
of emitting it.

The error is not recoverable and the only way to handle it is to
override the method.

Refs: #31818 (review)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Make virtual methods throw an ERR_METHOD_NOT_IMPLEMENTED error instead
of emitting it.

The error is not recoverable and the only way to handle it is to
override the method.

Refs: nodejs#31818 (review)
@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. labels Feb 22, 2020
@lpinca lpinca added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 22, 2020
@lpinca
Copy link
Member Author

lpinca commented Feb 22, 2020

cc: @nodejs/streams @ronag

@lpinca lpinca requested a review from ronag February 22, 2020 13:29
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@benjamingr
Copy link
Member

LGTM with semver-major and citgm

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 23, 2020

@lpinca
Copy link
Member Author

lpinca commented Mar 6, 2020

lpinca added a commit that referenced this pull request Mar 7, 2020
Make virtual methods throw an ERR_METHOD_NOT_IMPLEMENTED error instead
of emitting it.

The error is not recoverable and the only way to handle it is to
override the method.

PR-URL: #31912
Refs: #31818 (review)
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@lpinca
Copy link
Member Author

lpinca commented Mar 7, 2020

Landed in 6f0ec79.

@lpinca lpinca closed this Mar 7, 2020
@lpinca lpinca deleted the throw/err-method-not-implemented branch March 7, 2020 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants