-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
switch handle
hook from positional arguments to named arguments
#959
Conversation
}); | ||
|
||
const hooks = get_hooks(user_hooks); | ||
const hooks = { |
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.
get_hooks
seemed to be an unnecessary indirection, and I've removed it while updating this.
Is there any way to make this more obvious to existing users, like print an error telling you that the contract changed if someone upgrades and uses the old contract? That could be removed later on. |
Maybe in dev mode we could check |
@pngwn mentioned in chat that adding more specific errors or warnings around breaking changes in beta would be unmanageable. I don't disagree with this. In beta, I don't feel much guilt about changing the API like this if it's an improvement, and at this stage I also don't foresee that terribly many issues being opened which we would then have to close and say 'read the changelog'. |
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
This resolves #958 by switching the
handle
hook from positional arguments to named arguments. It also updates the appropriate tests and documentation.Before submitting the PR, please make sure you do the following
Tests
pnpm test
and lint the project withpnpm lint
Changesets
pnpx changeset
and following the prompts