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

Subapp overrides all paths, even the ones from original app #209

Closed
talentlessguy opened this issue Dec 23, 2020 · 11 comments
Closed

Subapp overrides all paths, even the ones from original app #209

talentlessguy opened this issue Dec 23, 2020 · 11 comments
Labels
bug Something isn't working core Affects `@tinyhttp/app` - the tinyhttp core help wanted Extra attention is needed

Comments

@talentlessguy
Copy link
Member

Describe the bug

When a subapp is mounted on a path, all of the middlewares with the same path in the original app are ignored and return 404 because it's being handled by a subapp.

To Reproduce

import { App } from '@tinyhttp/app'
import serveStatic from 'serve-static'
import sirv from 'sirv'
import path from 'path'

const app = new App()

const subapp = new App()

subapp.get('/',(req, res) => res.send('hello world'))


app.use('/', sirv('public'))
app.use('/', subapp)

app.listen(3000)
$ curl localhost:3000
hello world
$ curl localhost:3000/hello.txt
Not Found

Expected behavior

If subapp couldn't find the handler with the request path, the original middleware should handle it instead.

$ curl localhost:3000
hello world
$ curl localhost:3000/hello.txt
I am a file

Versions

  • node: 15.4.0
  • @tinyhttp/app: 1.1.0

Additional context

#203

@talentlessguy talentlessguy added bug Something isn't working core Affects `@tinyhttp/app` - the tinyhttp core labels Dec 23, 2020
@talentlessguy talentlessguy pinned this issue Dec 23, 2020
@talentlessguy
Copy link
Member Author

Another noticeable bug is that subapps have incorrect req.originalUrl and other URL values

@talentlessguy talentlessguy added the help wanted Extra attention is needed label Dec 23, 2020
@talentlessguy
Copy link
Member Author

talentlessguy commented Dec 23, 2020

@calumk do you have any ideas how to improve subapp handling? In the old version of Polka it used handler.bind(null, req, res) but it introduces a lot of bugs

in the new version... I'm not sure what happens here:

https://github.com/lukeed/polka/blob/3b4d86630f96083a041634421d11d8b2818a8b34/packages/polka/index.js#L30-L55

@calumk
Copy link
Contributor

calumk commented Dec 23, 2020

@talentlessguy
Sorry, i'm not going to be much help assisting with this, your expertise in this area far outweighs mine.
It seems i'm good at finding the problems, but I am afraid I am not proficient enough to solve them!

@calumk
Copy link
Contributor

calumk commented Dec 23, 2020

@talentlessguy was tinyhttp originally based on polka?

@talentlessguy
Copy link
Member Author

@calumk oh no problem then

tinyhttp was inspired by Polka and I took some concepts from it but it doesn't directly depend on Polka

I've spent 2 hours trying to make it work similar to Polka but atm I fail at doing this... but I hope I fix it

So yeah... subapps currently are broken

@talentlessguy
Copy link
Member Author

talentlessguy commented Dec 23, 2020

@calumk could you please try to install a new version?

$ npm i @tinyhttp/app@1.1.1

the tests seem to be passing

@calumk
Copy link
Contributor

calumk commented Dec 23, 2020

@talentlessguy - Thats seems to fix it for me 👍

Sorry for taking so many hours of your time!

@talentlessguy
Copy link
Member Author

@calumk yaaay

and lol you don't have to apologize for me making bugs

thanks for reporting it :D

@calumk
Copy link
Contributor

calumk commented Dec 23, 2020

You can add me to the bug contributors list ;)

@talentlessguy
Copy link
Member Author

@all-contributors add @calumk for bug

@allcontributors
Copy link
Contributor

@talentlessguy

I've put up a pull request to add @calumk! 🎉

@talentlessguy talentlessguy unpinned this issue Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core Affects `@tinyhttp/app` - the tinyhttp core help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants