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

fix: Ratelimit headers empty while running on Bun v1.0.x #418 #419

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

MikkelSVDK
Copy link
Contributor

Related to #418

Copy link
Member

@nfriedly nfriedly left a comment

Choose a reason for hiding this comment

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

This looks good, I think we can merge it in as-is.

However, to ensure it doesn't regress, do you think you can add automated tests in bun? Ideally run the full suite of unit tests in bun, but if that turns out to be unfeasible then perhaps just a few bun-speciffic ones, at least something that would catch this specific issue. Could be part of this PR or in a separate one.

@nfriedly
Copy link
Member

nfriedly commented Nov 5, 2023

Oh, and then npm run format will fix most lint issues automatically.

@MikkelSVDK
Copy link
Contributor Author

Yes, i'll make a new PR with some tests. However im not quite sure what scheme i should follow for the tests. Should I create a folder test/bun/*.test.ts or just put them with the others, since jests only looks for *-test.?

Bun only has one requirement Tests need ".test", "_test_", ".spec" or "_spec_" in the filename

@nfriedly
Copy link
Member

nfriedly commented Nov 6, 2023

I think it's possible to tweak the jest config to use whatever format we want. If we renamed the existing test, would they work in bun?

@nfriedly nfriedly merged commit 9d08a03 into express-rate-limit:main Nov 6, 2023
13 checks passed
@nfriedly
Copy link
Member

nfriedly commented Nov 6, 2023

OK, this will get published in v7.1.4 shortly. You might also want to file a bug on bun, since they claim full compatibility with the node:http module, and it defines setHeader as accepting any value.

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