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

feat(utils): replace fastify and express with tinyhttp and friends #7050

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

talentlessguy
Copy link
Contributor

@talentlessguy talentlessguy commented Feb 20, 2025

🎉 Thanks for submitting a pull request! 🎉

Summary

Replaces express, express-loggin, fastify and fastify-static with tinyhttp, @tinyhttp/logger, sirv and milliparsec

See e18e/ecosystem-issues#156


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@talentlessguy talentlessguy changed the title feat(utils): replace fastify with tinyhttp + sirv feat(utils): replace fastify and express with tinyhttp + sirv Feb 20, 2025
@talentlessguy talentlessguy changed the title feat(utils): replace fastify and express with tinyhttp + sirv feat(utils): replace fastify and express with tinyhttp and friends Feb 22, 2025
Copy link
Collaborator

@serhalp serhalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌🏼 Thanks for taking this on! I've taken a first pass at reviewing it. Could you please give this a quick rebase and take a look at my comments? I haven't had time to debug your integration tests yet, but I'll get to it soon.

await server.listen({ port: settings.frameworkPort })
const [address] = server.addresses()

const server = createServer((req, res) => app.handler(req as Request, res as Response))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type assertions are unsafe. Why is there a mismatch here? How can we resolve it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tinyhttp's Request and Response extend node:http's Request and Response. Probably could pass as a generic to createServer but if I recall correctly, it throws a type error

Copy link
Contributor Author

@talentlessguy talentlessguy Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a TypeScript issue with extension depth. Request extends IncomingMessage, which itself extends Stream. TypeScript thinks that Request doesn't extend Stream because of that, even though it does

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

Successfully merging this pull request may close these issues.

2 participants