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: extend mouse event extension methods #1427

Closed

Conversation

Qwertyluk
Copy link

Pull request description

Adds more Mouse Events extension methods.
Currently, it's impossible to call asynchronous overloads of mouse events without having to provide a MouseEventArgs parameter. In this PR, I've introduced a way to do so.

PR meta checklist

  • Pull request is targeted at main branch for code
    or targeted at stable branch for documentation that is live on bunit.dev.
  • Pull request is linked to all related issues, if any.
  • I have read the CONTRIBUTING.md document.

Code PR specific checklist

  • My code follows the code style of this project and AspNetCore coding guidelines.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have updated the appropriate sub section in the CHANGELOG.md.
  • I have added, updated or removed tests to according to my changes.
    • All tests passed.

@Qwertyluk
Copy link
Author

@dotnet-policy-service agree

@egil
Copy link
Member

egil commented Mar 28, 2024

Hey there. I'll take a look at this after Easter.

@Qwertyluk
Copy link
Author

Okay, no rush

@linkdotnet
Copy link
Collaborator

@egil As a side note:
Do we wish to continue to have the async overloads for v2?
I do feel it is more consistent not to have them anymore (as you don't have RenderAsync, which would await OnInitAsync and so on). So bUnit always begins something and if you want to await a state you resort to WaitForXXX.

Just my two cents - otherwise the PR looks good so far.

Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Sorry, took a bit to get to this. Look's good to me.

@linkdotnet we may want to consider creating a source generator for these at some point. Lots of boilerplate code here :)

@egil
Copy link
Member

egil commented Apr 9, 2024

Do we wish to continue to have the async overloads for v2?
I do feel it is more consistent not to have them anymore (as you don't have RenderAsync, which would await OnInitAsync and so on). So bUnit always begins something and if you want to await a state you resort to WaitForXXX.

I have not through this through. IIRC, awaiting the trigger allows users to await the event handler, not the actual rendering, so it may be worthwhile to keep them.

@Qwertyluk what is your scenario for using the async overloads instead of the non async ones?

Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Forgot to mention that you should add a section to the CHANGELOG.md file following the format we use about this change.

@Qwertyluk
Copy link
Author

Do we wish to continue to have the async overloads for v2?
I do feel it is more consistent not to have them anymore (as you don't have RenderAsync, which would await OnInitAsync and so on). So bUnit always begins something and if you want to await a state you resort to WaitForXXX.

I have not through this through. IIRC, awaiting the trigger allows users to await the event handler, not the actual rendering, so it may be worthwhile to keep them.

@Qwertyluk what is your scenario for using the async overloads instead of the non async ones?

Based on the posts above, I assume these changes won't address my issue.
My scenario was about using the non-async versions of event handlers (Click() etc.), which resulted in a component that hadn't re-rendered on time.
After reviewing the implementation of these event handlers, I realized they simply call their async counterparts, and I had no opportunity to await them.
However, after reading your posts, am I right to conclude that the only way to ensure the component has been re-rendered after the action is by using WaitForXXX and awaiting these event handlers can't guarantee this?

@linkdotnet
Copy link
Collaborator

Do we wish to continue to have the async overloads for v2?
I do feel it is more consistent not to have them anymore (as you don't have RenderAsync, which would await OnInitAsync and so on). So bUnit always begins something and if you want to await a state you resort to WaitForXXX.

I have not through this through. IIRC, awaiting the trigger allows users to await the event handler, not the actual rendering, so it may be worthwhile to keep them.
@Qwertyluk what is your scenario for using the async overloads instead of the non async ones?

Based on the posts above, I assume these changes won't address my issue. My scenario was about using the non-async versions of event handlers (Click() etc.), which resulted in a component that hadn't re-rendered on time. After reviewing the implementation of these event handlers, I realized they simply call their async counterparts, and I had no opportunity to await them. However, after reading your posts, am I right to conclude that the only way to ensure the component has been re-rendered after the action is by using WaitForXXX and awaiting these event handlers can't guarantee this?

Basically yes. The await will just wait for the event handlers code to finish - but that doesn't necessarily translate to "rendering is done". There still could be something in the render-pipeline.
For example because of the click you change a parameter that gets passed to a child component. That child component asynchronously (inside OnParametersSetAsync) will fetch something new via an API or so.
That part will not be awaited.

@Qwertyluk
Copy link
Author

Although these changes don't address my initial problem, I've updated the changelog accordingly.

@egil
Copy link
Member

egil commented Apr 12, 2024

Thanks @Qwertyluk.

You may want to rebase your branch on main. The validate-docs check that fails should be fixed on main.

@egil
Copy link
Member

egil commented Apr 12, 2024

Although these changes don't address my initial problem, I've updated the changelog accordingly.

With this in mind, I think it makes more sense to park this PR for now. We discussed this in our https://www.youtube.com/watch?v=_nStHkIujqc bUnit dev stream today, and I think we want to make these changes more generally to all trigger methods, if at all.

@egil egil closed this Apr 12, 2024
@Qwertyluk
Copy link
Author

I understand your decision.
I have one more question: If there are plans to make more general changes to the trigger methods, does it make sense to address issue #838 now?

@egil
Copy link
Member

egil commented Apr 15, 2024

@Qwertyluk I think we will keep the current pattern for v1 as is. We have a limited time to work on bUnit, but for v2 we may reimplement the trigger methods as a source generator so all current and future triggers are automatically available.

IF you want to submit a PR which solves #838 in v1 you are very welcome.

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.

3 participants