-
-
Notifications
You must be signed in to change notification settings - Fork 747
refactor: remove setting req.body to undefined #602
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
base: master
Are you sure you want to change the base?
refactor: remove setting req.body to undefined #602
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! Personally, I'm in favor of removing it completely, but let's wait to hear what the others think. I'm not entirely sure if this would be considered a breaking change, since some users might be relying on this behavior.
|
Hey @UlisesGascon what do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'm neutral on this, i'd need to do a bit more research to see if this validation is really necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @Phillip9587 perspective, but I will love to heard @wesleytodd perspective on this too :)
Reasoning: the various parsers should not modify the request object until it is clear it should be applied. Setting a property, even to undefined, may affect assumptions elsewhere. For example, `'req' in body` would be truthy even if the rest of the middleware was not applied.
It can be removed entirely since all tests are still passing without it. However, I recognize that some third-party consumers may still need it to be set.
In either case, the current version is not compatible with, for example, tRPC v11 when using FormData. So without this change, the workaround is to add a condition to avoid applying the middleware at all. Example:
```js
const json = express.json();
app.use((req, res, next) => {
if (req.url.includes('trpc')) return next();
return json(req, res, next);
});
```
61ae902 to
a5fa2c6
Compare
|
Updated: rebased to resolve conflicts and changed it to remove the lines instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said I like this change, but it’s a breaking one and should go out in a major release since users may rely on the current behavior. @jonchurch what do you think?
Reasoning: the various parsers should not modify the request object until it is clear it should be applied. Setting a property, even to undefined, may affect assumptions elsewhere. For example,
'req' in bodywould be truthy even if the rest of the middleware was not applied.It can be removed entirely since all tests are still passing without it. However, I recognize that some third-party consumers may still need it to be set.
In either case, the current version is not compatible with, for example, tRPC v11 when using FormData. So without this change, the workaround is to add a condition to avoid applying the middleware at all. Example: