lukeed / polka

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

Order of sub-applications affects sub-app routes on 1.0.0-next.11 #141

Closed kevinfiol closed 4 years ago

kevinfiol commented 4 years ago

Hey there, first off, wonderful library you got here!

I'm using 1.0.0-next.11 to build a small app. My app is structured like this:

// app.js
const app = polka();

import IndexRoutes from './routes/IndexRoutes.js';
import UserRoutes from './routes/UserRoutes.js';

app.use('/', IndexRoutes);
app.use('/user', UserRoutes);
// routes/IndexRoutes.js
const router = polka();
router.get('/', (req, res) => res.send(200, 'index'));

export default router;
// routes/UserRoutes.js
const router = polka();
router.get('/', (req, res) => res.send(200, 'user'));

export default router;

When I navigate to localhost:8080/ in my browser, surely enough I get a page that shows 'index'. However, navigating to localhost:8080/user returns Not Found.

I found I could easily fix this by simply re-ordering the app.use lines in app.js so that my subapplication under / comes last:

// app.js
const app = polka();

import IndexRoutes from './routes/IndexRoutes.js';
import UserRoutes from './routes/UserRoutes.js';

app.use('/user', UserRoutes);
app.use('/', IndexRoutes); // put this one last

Not sure if this is intentionally designed this way.

madacol commented 4 years ago

I was having similar issues, using the next version too. Look at this thread https://github.com/lukeed/polka/issues/87#issuecomment-471678832

It seems the next/^1.0.0 version has breaking changes with latest in this regard, so the docs middleware-sequence do not apply to the polka version we're using.

So as I understand it, as of version 1.0.0, polka's middleware sequence works like express, in order of declaration

kevinfiol commented 4 years ago

@madacol Thanks, this is a great explanation. I imagine the docs will reflect this once 1.0.0 is released.

lukeed commented 4 years ago

Attaching a route group with a a URL prefix (eg, use('/user', UserRoutes)) directs all incoming requests that begin with "/user/" to that sub-application. In this example, that's fine and is easy to reason about.

But the same thing applies for use('/', IndexRoutes) – anything that starts with / is directed towards that IndexRoutes sub-application. And since it's mounted first, it's the receives the request first. (As pointed out, a breaking change in the next version is that Polka dropped the "tiered" approach & processes route matches in their original order, like Express.) In this case, IndexRoute is another Polka application – it only have GET / defined and so anything else will return the 404 Not Found as described.

My recommendation is to move index/root-level routes to the root/main application:

polka()
  .use('/user', UserRoutes)
  .get('/', (req, res) => res.end('index'))

TBH, part of the reason why I still have this stuff behind a next tag is I'm not entirely sure how far I want to go down the Express routing-rabbit-hole. It's slow & inefficient by design – but I think in this particular case, it'd match the expectations of your original approach.

Closing for now, since this is intentional