Is modifying the request object in express.js bad practice?

user_4927377 0
Viewed 664

I joined a small team of devs for a start up. We have not even launched yet. I have been handed a backend service written in node/express. I have not worked with this tech beyond small pet projects. I was looking into implementing a style guide just to keep code consistent, with the goal of implementing this across other backend services as well.

That brought me to the Airbnb style guide. This part jumped out at me. Never mutate parameters

// bad
function f1(obj) {
  obj.key = 1;
}

// good
function f2(obj) {
  const key = Object.prototype.hasOwnProperty.call(obj, 'key') ? obj.key : 1;
}

In express there are typically controllers that get defined like so:

async function someController(req, res, next) {

  // I've seen similar code to this
  req.someNewProp = "Some new value."

  res.status(200).json({"someJSONKey":"someJSONVal"});
}

Middleware typically gets defined like this:

// Route
router.get('/endpoint', function1, function2)

async function function1(req, res, next) {

  // I've seen similar code to this
  req.someNewProp = "Some new value."

  // Pass req and res to function2
  next();
}

I notice that the req object, as it gets passed around gets modified a lot. Data gets added to this object in middleware and other functions as it is passed along before the response is returned. The original dev that authored the code referred to it as "keeping things in request scope." But that seems to directly contradict a major point in the style guide and made me wonder if this is bad practice.

So the question now is, is there a "better" or more widely accepted way to keep track of things in the context of the request that is not mutating the original request object? What are some approaches of doing this?

express node.js javascript
3 Answers

Express provides a name space for applications to store request/response processing variables by adding them as properties of res.locals. This seems a better choice than attaching not standard properties to the request or response objects themselves.

In similar fashion, global application variables can be stored as properties of app.locals

Unfortunately there doesn't seem to be a locals property defined for router instances. I have placed a reference to global router instance options in res.locals as the first middleware step in a route I wrote, but that was my choice.

It can happen that request properties do need to be changed during processing, such as req.path, but this is not something to avoid at all costs. For example Express provides req.originalURL so you can recalculate path components any time you need to by deliberate design.

You may find Express gets more interesting with use - I've only recently learned of and passed an error object argument to the next function. As for the Airbnb guide quote in the post: underwhelming in a word! The "good" and "bad" code quoted in the post don't do the same thing.

user_52171420

It's not a bad practice, this is the idea behind the middleware in express, in the simple definition, middlewares are functions that can modify the request and response object or even decide if the flow of the request continue or it's terminated. However you have to be careful and don't set a value in a pre-existing property or you can have some strange behaviors, also if the information that you are going to store in the request in big, you can think in other strategies for instance store the information in a memory database as Redis.

user_124329030

After reviewing the express docs I found this bit in the middleware section:

Middleware functions can perform the following tasks:

  1. Execute any code.
  2. Make changes to the request and the response objects.
  3. End the request-response cycle.
  4. Call the next middleware function in the stack.

Middleware Docs

So it's probably safe to say that if the docs explicitly say that we can modify req and response objects in middleware, it is probably not bad practice.

user_49273770
Related