lukeed / polka

A micro web server so fast, it'll make you dance! :dancers:
MIT License
5.43k stars 174 forks source link

Sub-app not working when mounted on "/" #107

Open waspeer opened 5 years ago

waspeer commented 5 years ago

I was trying to mount a sub-app on the root ("/") but it was throwing an error:

/node_modules/polka/index.js:95
        let loop = _ => res.finished || (i < len) && arr[i++](req, res, next);
                                                             ^

TypeError: arr[(i++)] is not a function

I tracked the error back to the .use() function of polka:

use(base, ...fns) {
        if (typeof base === 'function') {
            this.wares = this.wares.concat(base, fns);
                  // SUB-APP IS NOT HANDLED WELL BECAUSE OF THIS CONDITIONAL:
        } else if (base === '/') { 
            this.wares = this.wares.concat(fns);
        } else {
            base = lead(base);
            fns.forEach(fn => {
                if (fn instanceof Polka) {
                    this.apps[base] = fn;
                } else { ... etc ....

I have been trying to find a temporary fix without succes. Or is there another way to go about this?

lukeed commented 5 years ago

Hey, sorry for the delay – on vacation right now 🏖

Mounting via .use('/', ...) is identical to .use(...). That's the same in Express too.

This error, while not a super helpful message on its own, usually means that there's a problem with one of the functions you're trying to assign. Perhaps it's not a function at all.

Can you post a snippet containing the use() block and the functions you're trying to add? Or a reproduction repo would be good too.

Thanks

waspeer commented 5 years ago

Thanks for your reply! Funnily enough I’m leaving on vacation too right now, but I’ll send you some code as soon as I’m back next week.

--
Wannes Salomé

Op 28 augustus 2019 om 21:26:58, Luke Edwards (notifications@github.com(mailto:notifications@github.com)) schreef:

Hey, sorry for the delay – on vacation right now 🏖

Mounting via .use('/', ...) is identical to .use(...). That's the same in Express too.

This error, while not a super helpful message on its own, usually means that there's a problem with one of the functions you're trying to assign. Perhaps it's not a function at all.

Can you post a snippet containing the use() block and the functions you're trying to add? Or a reproduction repo would be good too.

Thanks

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub(https://github.com/lukeed/polka/issues/107?email_source=notifications&email_token=AC2LK436OMBDADBF3UQOVBLQG3GQFA5CNFSM4IQD4AZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5MGVKA#issuecomment-525888168), or mute the thread(https://github.com/notifications/unsubscribe-auth/AC2LK43EAGHMMYJB7EOXHC3QG3GQFANCNFSM4IQD4AZQ).

lukeed commented 5 years ago

Haha everyone is gearing up for the end-of-year sprint. Enjoy!

lukeed commented 5 years ago

Just checking in on this – still an issue?

waspeer commented 5 years ago

Hi! Sorry for being slow. I just ran another test and this still seems to be an issue. This is the code I tested it with:

// INDEX.JS
const polka = require("polka");

const { PORT = 3000 } = process.env;

polka()
  .use(require("./sub"))
  .listen(PORT, err => {
    console.log("Listening!");
  });
// SUB.JS
const polka = require("polka");

module.exports = polka()
  .get("/", (req, res) => {
    res.end("r00t");
  })
  .get("/:name", (req, res) => {
    res.end(`Hello there ${req.params.name}!`);
  });

This time the error is a little different:

/node_modules/polka/index.js:7
    return x.charCodeAt(0) === 47 ? x : ('/' + x);
             ^

TypeError: x.charCodeAt is not a function

The error dissappears when change .use(require("./sub")) to something like .use("sub", require("./sub")).

Hope this helps!

matthewrobb commented 5 years ago

I, too, am having this same problem exactly.

lukeed commented 5 years ago

This is fixed in next and I'm not sure if it's worth backporting at this point.

As mentioned, the easy workaround is to use('/', ...) instead, which achieves exactly the same intended result.

waspeer commented 5 years ago

Hi!

For me using use('/', ...) also throws an error. The error I mentioned when I opened the issue. But to elaborate.

// INDEX.JS
const polka = require("polka");

const { PORT = 3000 } = process.env;

polka()
  .use("/", require("./sub"))
  .listen(PORT, err => {
    console.log("Listening!");
  });
// SUB.JS
const polka = require("polka");

module.exports = polka()
  .get("/", (req, res) => {
    res.end("r00t");
  })
  .get("/:name", (req, res) => {
    res.end(`Hello there ${req.params.name}!`);
  });

This will start running and log Listening!, but when I try to visit localhost:3000 I get the following error:

    let loop = _ => res.finished || (i < len) && arr[i++](req, res, next);
                                                             ^

TypeError: arr[(i++)] is not a function

This is the error I mentioned when I opened the issue.

RomiC commented 5 years ago

Faced with the same issue. A small snippet below:

const polka = require('polka');

polka()
  .use('/', polka().use('/', (req, res) => res.end('sub-app response')))
  .listen(3000);
lukeed commented 5 years ago

Hey guys, sorry for the delay.

I took a look at this a few times & have come to the conclusion that it's not worth fixing on the current stable branch. In order to fix it correctly, it requires breaking changes that are already part of the 1.0 work (@next).

I will add complete testing for this (it's not fully implemented, yet) and update this issue once a new release has gone out.

Thanks for you patience :)

lukeed commented 5 years ago

Available with polka@1.0.0-next.7 via npm install polka@next 👍