Skip to content

Conversation

@H4ad
Copy link
Collaborator

@H4ad H4ad commented Oct 11, 2025

I didn't split the commits correctly since I did a couple changes that I wanted to discuss first.

So:

  • Some types were incorrect, I tried to fix all of them
  • Added export for the memory plugin
  • Fixed the memory plugin since it was in the old behavior
  • Changed how to specify a plugin (class instead of instance)
    • This is a breaking change compared to the old behavior but this will ensure the memory plugin and many other plugins that rely on state per benchmark works correctly.

Copy link
Owner

@RafaelGSS RafaelGSS left a 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.

@H4ad
Copy link
Collaborator Author

H4ad commented Oct 16, 2025

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.

@RafaelGSS
Copy link
Owner

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 .reset after each benchmark fn.

@RafaelGSS
Copy link
Owner

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).

@H4ad H4ad force-pushed the feat/memory-plugin branch 3 times, most recently from 1ff42a0 to 4c646e4 Compare October 22, 2025 02:37
@H4ad
Copy link
Collaborator Author

H4ad commented Oct 22, 2025

our reponsability to .reset after each benchmark fn.

There's no reset. But I added again and kept the instance way of having plugins.

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).

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)
Copy link
Collaborator Author

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?

@H4ad H4ad force-pushed the feat/memory-plugin branch 2 times, most recently from 433f7c7 to b256419 Compare October 22, 2025 02:43
@H4ad H4ad force-pushed the feat/memory-plugin branch from b256419 to a74f090 Compare November 2, 2025 23:31
@RafaelGSS RafaelGSS merged commit cd4f96e into RafaelGSS:main Nov 3, 2025
5 of 6 checks passed
@H4ad H4ad deleted the feat/memory-plugin branch November 3, 2025 12:46
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