-
Notifications
You must be signed in to change notification settings - Fork 83
refactor: write tests to also work with deno and bun #1143
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
Conversation
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just a nit
test/helpers/find-free-port.ts
Outdated
tries++; | ||
} | ||
throw new Error("No available ports"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can add https://github.com/sindresorhus/get-port as a devdependency instead of adding this code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could but the problem is, that we run into race conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how but I won't die on my hill 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D
const result = concatUint8Array(data); | ||
// Switch to queue microtask when we want to support bun and deno | ||
setImmediate(resolve, result); | ||
queueMicrotask(() => resolve(result)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, just give the PR a proper title explaining what you've done. You can use refactor: ...
as prefix.
@gr2m acceptable? |
🎉 This PR is included in version 14.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 13.9.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
My first goal was to ensure, that the "webmiddleware" is tested properly. But then I realized, that the web middleware is kind of better than the old node logic. So that was used as base for the unification.
So anyhow. I tried first tried to get it work with Deno. So tried to remove node specific code as much as possible. So instead of using Buffer I used Uint8Array.
I basically needed to rewrite all the unit tests. Why keeping vitest?
node --test
works only reliable with node 24. But I think it is now very cool, as the tests run additionally on deno and bun.