CODEWITHSUNDEEP
javascriptnode.jsexpressexpress-router
Just4you
Just4you

Reputation: 66

Express node.js forEach route

I was trying to make a routes for each ID I using a forEach loop but It stay loading until timeout reaches, all expected values are in place, all good but the second route is not running, I was fighting it despretly until now. I made sure there is a problem.

server.js

const router = require('express').Router();

function isAuthorized(req, res, next) {
    if (req.user) {
        next();
    }
    else {
        res.redirect('/login')
    }
}

let myguild = [];

router.get(`*`, isAuthorized, (req, res) => {
    res.status(200);
    console.log("wow");
    console.log(req.user.guilds.length)
    req.user.guilds.forEach(guild => {
        myguild.push(guild);
    })
    console.log("Finished");
    myguild.forEach(guild => {
        console.log('Started')
        router.get(guild.id, (req, res) => { // here is the problem
            console.log("uh")
            res.send("HAMBURGER")
            console.log(req, res, guild)
        })
        console.log("Outed")
    })

});

module.exports = router;

output:

wow
23
Finished
Started
Outed
Started
Outed
Started
Outed
Star... 'there is more but this is enough'

It should behave and run within server/${guild.id} but got (failed) request

Any Ideas?

Upvotes: 0

Views: 1245

Answers (3)

Just4you
Just4you

Reputation: 66

Thanks for everyone helped.

Inspired answer

Final Result:

server.js

router.get('/:guildid', isAuthorized, (req, res, next) => {
    console.log('started')
    const guildid = req.params.guildid
    if (req.user.guilds.some(guild => guild.id === guildid)) {
        console.log('uh')
        res.send("HAMBURGER").end()
    } else {
        res.sendStatus(404);
    }
})

Upvotes: 1

deezy
deezy

Reputation: 515

You might need to redesign the API to better fit what you're trying to accomplish. If you already know which guilds are available then you'd need to create those before the server is initialized.

Even if they come from a database or are dynamic, you can loop through the guild "options" and create endpoints then provide access to them only if the user is qualified.

const { guilds } = require('./config')
const guildHandler = (req, res) => {
    // Assuming you're doing more here
    res.send('Hamburger')
}

guilds.forEach(guild => router.get(`/guilds/${guildId}`, guildHandler)

Or if you are NOT doingg something different in the middleware for each guild then you could just have a single route for guild.

router.get('/guilds/:guildId, guildHandler)

Not really sure what you're trying to accomplish but checkout out the Express docs. They solve most use cases fairly easily.

https://expressjs.com/en/api.html#req

Upvotes: 2

O. Jones
O. Jones

Reputation: 108851

You never call res.end() from your outer res.get() handler, so the request never completes.

And, with respect, creating route handlers like that in a loop is a mistake. It will lead to real performance trouble when your app gets thousands of guilds.

You'll want to use just one route, with a named route parameter, something like this.

const createError = require('http-errors')

router.get(':guildid', isAuthorized, (req, res, next) => {
  const guildid = req.params.guildid
  if (req.user.guilds.includes(guild)) { 
        console.log("uh")
        res.send("HAMBURGER").end()
        console.log(req, res, guildid)
   } else {
     next(createError(404, guildId + ' not found'))
   }
})

Upvotes: 1

Related Questions