Skip to content

refactor: move req.body = undefined to just before read attempt #602

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Conversation

smonn
Copy link

@smonn smonn commented Apr 4, 2025

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, 'body' in req would be truthy even if the rest of the middleware was not applied.

Arguably, it can be removed entirely since all tests are still passing without it. But keeping it around in case there are other historical reasons behind it.

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:

const json = express.json();
app.use((req, res, next) => {
  if (req.url.includes('trpc')) return next();
  return json(req, res, next);
});

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.

Arguably, it can be removed entirely since all tests are still passing without it. But keeping it around in case there are other historical reasons behind it.

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);
});
```
Copy link
Contributor

@Phillip9587 Phillip9587 left a 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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants