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

Is this usecase recommended or discouraged? #3148

Closed
franciscop opened this issue Dec 9, 2016 · 3 comments
Closed

Is this usecase recommended or discouraged? #3148

franciscop opened this issue Dec 9, 2016 · 3 comments

Comments

@franciscop
Copy link

I created some modules for normalizing URLs in different ways (+ there are some others out there), so I would like to bundle them into a single module. This works right now:

app.use((req, res, next) => {
  req.app.use((req, res, next) => {
    console.log("I am called!");
    next();
  });
  req.app.use((req, res, next) => {
    console.log("Me too!");
    next();
  });
  console.log("Great");
  next();
});

Which correctly outputs:

Great
I am called!
Me too!

However I worry that this is abusing express functionality/original intent. So I'd like to ask if this is frowned upon or perfectly valid within express' API.

@blakeembrey
Copy link
Member

blakeembrey commented Dec 9, 2016

This is not a good way to do it, you're creating a memory leak by mounting applications every single request loop which means that for each request, you have one extra set of middleware mounted (and a new function for each middleware instance too == more memory overhead).

It sounds like what you want to do is to look up how Express routers work. It allows you to do this:

const router = express.Router();

router.use((req, res, next) => {
  console.log("I am called!");
  next();
});

router.use((req, res, next) => {
  console.log("Me too!");
  next();
});

app.use(router);

Basically, you can create any instance you'd like and accept (req, res, next) - inside that you can do whatever you'd like - be that routing, request management, etc. I wouldn't mess with the application instance itself as you will create a memory leak adding too many instances of middleware.

@dougwilson
Copy link
Contributor

And to build on what @blakeembrey said, from a strict sense of the question "So I'd like to ask if this is frowned upon or perfectly valid within express' API.": all of that is valid use of the Express API that is public & supported, though it does seem odd.

@franciscop
Copy link
Author

franciscop commented Dec 10, 2016

@blakeembrey that makes total sense, of course the "base" use one is also a middleware so it'll be called each time. A flag or counter could be added to avoid leaking, but then your solution with the Router seems way better so I'll use it that way. Thanks!

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

No branches or pull requests

3 participants