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

feat: add mode to fn hooks #259

Merged
merged 4 commits into from
Mar 18, 2025
Merged

feat: add mode to fn hooks #259

merged 4 commits into from
Mar 18, 2025

Conversation

crimx
Copy link
Contributor

@crimx crimx commented Mar 17, 2025

Provide a way to check the mode for fn hooks that is consistent with setup and teardown.

@jerome-benoit
Copy link
Collaborator

jerome-benoit commented Mar 17, 2025

The idea and initial implementation are ok, but lack:

  • existing JSDoc update
  • documentation update
  • dedicated unit tests testing that callbacks mode is respected

Since it's a breaking change in the current PR implementation, a backward compatible implementation should be investigated.

@crimx
Copy link
Contributor Author

crimx commented Mar 18, 2025

The idea and initial implementation are ok, but lack:

  • existing JSDoc update
  • documentation update
  • dedicated unit tests testing that callbacks mode is respected

Since it's a breaking change in the current PR implementation, a backward compatible implementation should be investigated.

  • I noticed the JSDoc artifacts are in the source code. Should I build the JSDoc manually? That would be hard to review.
  • There is not existing documentation to update.
  • Tests added, just like setup and teardown.

Copy link

pkg-pr-new bot commented Mar 18, 2025

Open in Stackblitz

npm i https://pkg.pr.new/tinylibs/tinybench@259

commit: 7409c17

@crimx
Copy link
Contributor Author

crimx commented Mar 18, 2025

I don't see any breaking change? The callbacks are backward compatible. It is unlikely that user passed in a callback with an optional parameter.

@jerome-benoit
Copy link
Collaborator

jerome-benoit commented Mar 18, 2025

  • I noticed the JSDoc artifacts are in the source code. Should I build the JSDoc manually? That would be hard to review.

Since the existing code is not documented, only the public API change should have a JSDoc, so the FnHook type definition should have a typedoc compatible comment.
I will probably add a proper one on Hook type definition.

  • There is not existing documentation to update.

I guess a properly commented FnHook type definition should be enough.

@jerome-benoit
Copy link
Collaborator

I don't see any breaking change? The callbacks are backward compatible. It is unlikely that user passed in a callback with an optional parameter.

See #259 (comment)

@crimx
Copy link
Contributor Author

crimx commented Mar 18, 2025

I don't see any breaking change? The callbacks are backward compatible. It is unlikely that user passed in a callback with an optional parameter.

See #259 (comment)

You don't need to make mode optional. Parameters added in callbacks are backward compatible. Playground Link

@jerome-benoit
Copy link
Collaborator

See #259 (comment)

You don't need to make mode optional. Parameters added in callbacks are backward compatible. Playground Link

It will fail with strict type checking options enabled.

@crimx
Copy link
Contributor Author

crimx commented Mar 18, 2025

image

No I believe it won't. The type would be broken in these two scenarios:

  1. If users directly utilize the FnHook type for their definitions, but this type wasn't previously available, so it won't occur.
  2. If users provide callbacks with optional parameters, which is unlikely given the context of how opts is used.

But anyway I made the changes as requested, following the Hook signature.

crimx added 3 commits March 18, 2025 11:56

Verified

This commit was signed with the committer’s verified signature.
kdeldycke Kevin Deldycke

Verified

This commit was signed with the committer’s verified signature.
kdeldycke Kevin Deldycke
@jerome-benoit jerome-benoit merged commit e13f07a into tinylibs:main Mar 18, 2025
15 checks passed
@jerome-benoit
Copy link
Collaborator

Thanks !

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.

None yet

2 participants