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

Update getParameter example to use index instead of name #1056

Closed
wants to merge 1 commit into from

Conversation

mikeseese
Copy link

This PR updates the Benchmarker.js to use the most recent signature for HTTPRequest::getParameter, which accepts the index of the parameter instead of the name. The Router.js example already has the appropriate change, but this example is outdated.

@uNetworkingAB
Copy link
Contributor

It's the opposite - getParameter(name) is brand new, getParameter(index) is old as rock. But the whole file was taken from here https://github.com/SaltyAom/bun-http-framework-benchmark

@mikeseese
Copy link
Author

mikeseese commented May 14, 2024

Ah I see; how would you like to proceed as this library doesn't use the same implementation as Bun's (and there's inconsistency between this example and Router.js)? The failing tests seem to be unrelated to this change (primarily compiler errors in Http3Context.h).

@uNetworkingAB
Copy link
Contributor

I'm confused but I see the problem.

The JS wrapper does not expose the new getParameter(name) only the old getParameter(integer). The Benchmarker.js still happens to work because the integer value of "id" happens to be 0 as it fails to be an integer so it still gets the correct parameter.

What Bun has to do with this, I have no idea. The issue is that uWS.js needs to wrap the new overload that takes string.

@mikeseese mikeseese closed this May 22, 2024
@mikeseese mikeseese deleted the patch-1 branch May 22, 2024 05:14
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.

None yet

2 participants