jsakamoto / Toolbelt.Blazor.HotKeys2

This is a class library that provides configuration-centric keyboard shortcuts for your Blazor apps.
https://jsakamoto.github.io/Toolbelt.Blazor.HotKeys2/
Mozilla Public License 2.0
87 stars 6 forks source link

Ambiguity Error after upgrading to v3.3.0 #18

Closed fingers10 closed 3 months ago

fingers10 commented 3 months ago

After upgrading to v3.3.0, there comes and ambiguty error as shown

The call is ambiguous between the following methods or properties: 'HotKeysContext.Add(Key, Func, string, Exclude)' and 'HotKeysContext.Add(Key, Func, string, Exclude)' [C:\Repos\ilovedotnet\CommonComponents\CommonComponents.csproj]

Here is the sample details from dependabot PR - https://github.com/ILoveDotNet/ilovedotnet/actions/runs/8236097580/job/22521723548?pr=242#step:11:11

Any docs update on this and how to handle?

fingers10 commented 3 months ago

Ah I thought something wrong with the package.. I added explicit cast in my calling code and resolved this.

HotKeysContext = HotKeys.CreateContext().Add(Key.Slash, (Func<ValueTask>)(async () => await SearchInput.FocusAsync()));
robertmclaws commented 3 months ago

Yeah this is kind of not cool... there actually is something wrong with the package because this is a breaking change in a SemVer non-breaking release. You shouldn't need to cast in order to have this work. Would really appreciate it if this could be reopened, please.

Also, it might be a good idea to create a unit test to catch this kind of behavior.

fingers10 commented 3 months ago

Yeah this is kind of not cool... there is something wrong with the package. You shouldn't need to cast in order to have this work. Would really appreciate it if this could be reopened.

Also, it might be a good idea to create a unit test to catch this kind of behavior.

@jsakamoto what's your thoughts?

jsakamoto commented 3 months ago

@fingers10 I'm really sorry for making your build fail. I could not have predicted that v.3.3.0 would cause such a breaking. That's my fault.

By the way, I sent a pull request to simplify your code to register hotkeys. As you can see in that pull request from me, you can now implement the code to register hotkeys from:

HotKeysContext = HotKeys.CreateContext()
    .Add(Key.Slash, (Func<ValueTask>)(async () => await SearchInput.FocusAsync()));

to:

HotKeysContext = HotKeys.CreateContext()
    .Add(Key.Slash, () => SearchInput.FocusAsync());

The change in v.3.3.0 is aimed at simplifying the code to register hotkeys, like above. I believe v.3.3.0 maintains binary compatibility but does not maintain source code compatibility, which is different from my intention.

jsakamoto commented 3 months ago

@robertmclaws

Yeah this is kind of not cool... there actually is something wrong with the package because this is a breaking change in a SemVer non-breaking release.

Yes, you are right. That's my fault... In fact, I couldn't come up with the idea that v.3.3.0 includes source code incompatibility. I believed the change won't cause a breaking change at that time. That is why I incremented the minor version number, not the major number. Anyway, that's my fault.

Also, it might be a good idea to create a unit test to catch this kind of behavior.

I already have not only unit tests but also E2E tests to detect this kind of breaking. However, the test cases I have could not cover to detect the issue fingers10 reported. That's my fault...

Actually, I was concerned about whether the change on v.3.3.0 is safe for breaking change. So, I first released a preview version. But there was no issue report after two months, so I published it as an official release.

Robert, you are certainly right. I am terribly depressed about my failure and have no motivation to do anything else. I'm sorry for that.

P.S. To avoid further confusion, I'm not going to publish a new package with a new major version number.

fingers10 commented 3 months ago

@jsakamoto You always do a great work and contribute to community. Never ever get disappointed. We are all here to support you. It's okay if we make mistakes. No need to worry.

We are not blaming anyone. We are supporting each other by spotting issues which help community members.

Please don't stay away from posting major versions. I'll also try preview version and share observations and share feedback with you in future.

You are always a inspiration and motivation to me. Keep doing your great works man.

robertmclaws commented 3 months ago

I did not mean at all to make you feel bad man. This is an amazing library. Sometimes stuff happens, and method overloads can really screw with things in ways you don't expect. Has happened to me; happens to the best of us.

That's just why I was saying that there are probably ways to help catch something like that again in the future.

You're the man! Keep up the amazing work! 🤜🏻

jsakamoto commented 3 months ago

Thank you all very much for your kind words. Rather than that, I am sorry I made you post such a caring me.

Maybe it is because I have been tired for the past few days that this issue might make me so hard. I would like to get some more sleep and rest well. Then, I will be able to have a positive attitude again.

I apologize for the trouble I have caused you (that means not only this source code-level breaking change but also that I made you all care about me). Anyway, thank you all again.