Skip to content

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, '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:

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

Copy link
Member

@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.

@Phillip9587
Copy link
Member

Hey @UlisesGascon what do you think?

@bjohansebas bjohansebas requested a review from a team April 25, 2025 23:40
Copy link
Member

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

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

LGTM

@bjohansebas
Copy link
Member

I'm in favor of removing it completely,

I'm neutral on this, i'd need to do a bit more research to see if this validation is really necessary.

Copy link
Member

@UlisesGascon UlisesGascon left a 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 :)

@UlisesGascon UlisesGascon requested a review from wesleytodd May 9, 2025 13:05
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);
});
```
@smonn smonn force-pushed the simon/refactor-req-body-undefined-location branch from 61ae902 to a5fa2c6 Compare September 30, 2025 01:06
@smonn smonn changed the title refactor: move req.body = undefined to just before read attempt refactor: remove setting req.body to undefined Sep 30, 2025
@smonn
Copy link
Author

smonn commented Sep 30, 2025

Updated: rebased to resolve conflicts and changed it to remove the lines instead.

Copy link
Member

@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.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants