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

switch rpc.ts to itty-router instead of connect cors body-parser #3467

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

foufrix
Copy link
Contributor

@foufrix foufrix commented Jun 21, 2024

Following discussion in #3450

Attempt to move client/src/util/rpc.ts to itty-router to remove body-parser connect cors

It's not working now, from what I understood, we will need to add an adapter @whatwg-node/server, if someone can confirm that @holgerd77 @acolytec3

Documentation about that can be found here

Still trying to understand what I'm doing wrong here

@acolytec3
Copy link
Contributor

Sorry for not commenting sooner but if it turns it we need that whatwyg/node dependency for itty-router to work, we're not gaining anything as that brings in another 5-6 dependencies of it's own. I took an initial look and seems like itty-router might not be the right choice after all. I will take another look this week but we might want to look for alternatives that work with less hassle

@foufrix
Copy link
Contributor Author

foufrix commented Jul 10, 2024

Hey @acolytec3, sorry for the late reply. I was away.

I'm not 100% sure that we need it, but the adapter seems necessary to use the HTTP createServer method. I don't know if we can do without this HTTP createServer method that we pass to Node.

@acolytec3
Copy link
Contributor

Hey @acolytec3, sorry for the late reply. I was away.

I'm not 100% sure that we need it, but the adapter seems necessary to use the HTTP createServer method. I don't know if we can do without this HTTP createServer method that we pass to Node.

Understood. If that's the case, I don't think we should go with itty-router. The whole idea was to reduce our dependency subtree for connect/cors/body-parser. If there isn't something similar that doesn't require shims and additional deps to make it work, we might as well just stick with what we have now or internalize it (though I don't love that idea all that much).

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