-
-
Notifications
You must be signed in to change notification settings - Fork 50
fix: introduce and use BenchLike interface to decouple task from Bench. #422
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
commit: |
size-limit report 📦
|
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.
Pull Request Overview
This PR introduces an AbstractBench interface to improve type safety and refactors the Bench class to expose properties directly instead of through an internal opts object. This change addresses issue #53 by making the benchmark configuration immutable and improving the API design.
Key Changes:
- Introduced
AbstractBenchinterface defining the public contract for the Bench class - Refactored
Benchclass to expose configuration properties as readonly class members instead of throughbench.opts.* - Updated all test files to access properties directly (e.g.,
bench.timeinstead ofbench.opts.time)
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Adds AbstractBench interface, Concurrency and NowFn type aliases, and updates type definitions |
| src/bench.ts | Refactors Bench class to implement AbstractBench with direct property exposure instead of opts object |
| src/task.ts | Updates Task class to use AbstractBench interface and access properties directly |
| test/duration.test.ts | Updates tests to use bench.now() instead of bench.opts.now() |
| test/bench-consistency-sync.test.ts | Updates tests to use bench.iterations and bench.time instead of bench.opts.* |
| test/bench-consistency-async.test.ts | Updates tests to use bench.iterations and bench.time instead of bench.opts.* |
| test/sequential.test.ts | Updates tests to set concurrency via constructor options and removes dynamic property assignments |
| test/concurrency.test.ts | Splits single test into two separate tests and sets concurrency via constructor |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
43081j
left a comment
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.
makes sense to me
im not sure if Abstract makes sense though since i'd expect that to be an abstract class for example. but i have no better suggestions other than BenchLike 😅
|
Yes you are right. Abstract would be meaning abstract class. Hmm. And IBench sucks because why the I... |
|
renaming to BenchLike. Its an internal type but should not be confusing with abstract classes. |
should fix #53