Skip to content

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.

@pkg-pr-new
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.

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

2 participants