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: remove re-routing from handler #1847

Merged
merged 1 commit into from Jul 14, 2020
Merged

feat: remove re-routing from handler #1847

merged 1 commit into from Jul 14, 2020

Conversation

@ghermeto
Copy link
Member

@ghermeto ghermeto commented Jul 11, 2020

BREAKING CHANGE: deprecates and removes re-routing when passing a string parameter to next()

Pre-Submission Checklist

  • Opened an issue discussing these changes before opening the PR
  • Ran the linter and tests via make prepush
  • Included comprehensive and convincing tests for changes

Issues

Changes

Assumes the standard node.js callback API: cb(err, value) and treats next() as a callback. This way:

  1. deprecates and removes re-routing;
  2. next('foo') will be considered an error;
  3. the returned value from an async function is ignored (as it would for next(null, 'foo'));
BREAKING CHANGE: deprecates and removes re-routing when passing a
string parameter to `next()`
@ghermeto ghermeto requested a review from restify/current-core Jul 11, 2020
@mmarchini
Copy link
Contributor

@mmarchini mmarchini commented Jul 13, 2020

next('foo') will be considered an error;

The standard Node.js callback API accepts any value type for the first argument. Should we do the same?

@ghermeto
Copy link
Member Author

@ghermeto ghermeto commented Jul 14, 2020

next('foo') will be considered an error;

The standard Node.js callback API accepts any value type for the first argument. Should we do the same?

Yes, actually right now it does return an InternalServerError if you give call it next(1) or next({}). With strings, it was checking for that on the error handling and re-routing.

Copy link
Contributor

@mmarchini mmarchini left a comment

Ok, if it's consistent with the current behavior for other types I think it's reasonable. If needed we can make more changes later.

@ghermeto ghermeto merged commit 9153587 into master Jul 14, 2020
7 checks passed
7 checks passed
lint lint
Details
test node 10.x on ubuntu-latest
Details
test node 12.x on ubuntu-latest
Details
test node 14.x on ubuntu-latest
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk (Restify) No manifest changes detected in 1 project
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.