Skip to content

Conversation

ErickWendel
Copy link
Member

it changes the calls function from the callTracker API, its docs, and tests that use this module. This is a breaking change as the calls function was changed

Refs: #43161

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. needs-ci PRs that need a full CI run. labels Jun 7, 2022
@ErickWendel ErickWendel force-pushed the test/change-calltracker-calls-design branch 5 times, most recently from 07acc4d to ae31757 Compare June 7, 2022 13:39
@Trott
Copy link
Member

Trott commented Jun 7, 2022

Can you change the subsystem in the first line of the commit message from test: to assert:?

@Trott
Copy link
Member

Trott commented Jun 7, 2022

@nodejs/assert

it changes the callTracker API, its docs and tests that use this module

Refs: nodejs#43161
@ErickWendel ErickWendel force-pushed the test/change-calltracker-calls-design branch from ae31757 to a14d9bb Compare June 7, 2022 13:47
@ErickWendel
Copy link
Member Author

assert

uh! done!

@ErickWendel ErickWendel changed the title test: change callTracker.calls signature to use an object param instead of positional arguments assert: change callTracker.calls signature to use an object param instead of positional arguments Jun 7, 2022
#callChecks = new SafeSet();

calls(fn, exact = 1) {
calls({ fn = noop, exact = 1 } = { fn: noop, exact: 1 }) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unnecessary breaking change. The old signature should keep working, or must be deprecated first.

None of the existing tests should need to be modified.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed it here #43161 as the old signature is a bit confusing I thought it'd be better change from positional to object arguments.

This is my first time working on an experimental feature so I thought everything should be moved to the new signature. Even for experimental features we should keep the old tests?

Do you have an example of how to deprecate the old signature? I haven't seen people deprecating functions from experimental features so I'm a bit lost on it haha

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I forgot that CallTracker is still experimental. Still, it's been around since Node.js 12, and there is no reason to break every application that's using it.

I would maintain both signatures for now and then maybe deprecate the old signature at some point.

With that being said, unless we have specific options that we want to add later, I am not sure if this an improvement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh nice! Do you think it’s still valid to ads this change?

@benjamingr
Copy link
Member

Hey @ErickWendel let me know if you want feedback/help with this when you're here for NodeTLV.

In general I think what Tobias suggested is good basically:

  • Add the new call signature, deprecate the old call signature (I'm fine with showing a runtime deprecation warning when using it and skipping docs-only because it's experimental).
  • In a future semver-major PR, remove the old signature

@ErickWendel
Copy link
Member Author

Hey @ErickWendel let me know if you want feedback/help with this when you're here for NodeTLV.

In general I think what Tobias suggested is good basically:

  • Add the new call signature, deprecate the old call signature (I'm fine with showing a runtime deprecation warning when using it and skipping docs-only because it's experimental).
  • In a future semver-major PR, remove the old signature

Suure thank you!! I’ll try doing it by the next week. 🤩🤩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

assert Issues and PRs related to the assert subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants