-
-
Notifications
You must be signed in to change notification settings - Fork 17
Fixes on Types & Memory Plugin #113
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
RafaelGSS
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.
I don't think we should land breaking changes at this point (the change of plugins to not need new anymore is a breaking change). I'd focus this PR solely on plugins/memory.js.
|
We are on v0, so I don't think the breaking changes are really impactful. Also, how do we solve the issue of creating new instances for each benchmark when we can't instantiate it? The change from not need new, to need new was also a breaking change. |
|
The difference is that this package is now way more used than it was when I did the breaking change https://npmtrends.com/bench-node - I have personally installed it on some automations that I prefer not to break. Why do we want to create new instances for each benchmark? It's user's responsibility to create one instance for each suite, and our reponsability to |
|
Also, it would be great if we split this PR into different PRs. It makes my review easier (I know they are already split into commits). |
1ff42a0 to
4c646e4
Compare
There's no reset. But I added again and kept the instance way of having plugins.
I simplified the PR, so should be easier to review now. |
| 5. [Benchmark Modes](#benchmark-modes) | ||
| 1. [Operations Mode (Default)](#operations-mode) | ||
| 2. [Time Mode](#time-mode) | ||
| - [Install](#install) |
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 remember to update this part but I thikn it makes sense to update, so should I keep it?
433f7c7 to
b256419
Compare
b256419 to
a74f090
Compare
I didn't split the commits correctly since I did a couple changes that I wanted to discuss first.
So: