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

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 a team July 11, 2020 04:09
@mmarchini
Copy link
Contributor

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

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

Choose a reason for hiding this comment

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

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
@pinko-fowle
Copy link

pinko-fowle commented Sep 23, 2022

There's no replacement api is there? A request.forward or some such?

We're wondering what our migration forward will look like.

Also: the 8to9 guide explicitly mentions re-routing & calling next() with a string as a thing it supports.

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

5 participants