Skip to content

feat: Add bind function to simulate @bind directive#750

Merged
egil merged 18 commits intomainfrom
feature/bind
Jun 24, 2022
Merged

feat: Add bind function to simulate @bind directive#750
egil merged 18 commits intomainfrom
feature/bind

Conversation

@linkdotnet
Copy link
Copy Markdown
Collaborator

@linkdotnet linkdotnet commented Jun 9, 2022

Pull request description

Addresses and fixes #747

If the design is okay with you I would update the documentation and all necessary documents.

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.

@linkdotnet linkdotnet requested a review from egil June 9, 2022 11:53
@linkdotnet linkdotnet force-pushed the feature/bind branch 2 times, most recently from a9b49cb to 41f9cb2 Compare June 9, 2022 14:03
@linkdotnet
Copy link
Copy Markdown
Collaborator Author

I went ahead and added the documentation and stuff. So from my side everything is set and done.
Let me know your thoughts.

Copy link
Copy Markdown
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.

Thanks Steven. This is a good start!

I have added some suggestions in this review and played around with the code locally. There are some rough edges that needs sanding here, code docs needs to be more precise, and we have to make sure we handle edge cases where users do something unexpected. E.g. what if the user choses the ValueChanged property instead of the Value property in the parameterSelector.

A thing I would like us to get better at is writing good error/exception messages in more places, like we do with the missing JSInterop setup messages, that basically tells the user what is wrong, and what to do about it. The last part is a game changer for people, and should help them get things working more easily. The changes in this PR could be the first place in the component parameter builder we start doing that.

Comment thread docs/samples/tests/xunit/TwoWayBindingTest.cs Outdated
Comment thread src/bunit.core/ComponentParameterCollectionBuilder.cs Outdated
Comment thread docs/site/docs/providing-input/passing-parameters-to-components.md Outdated
Comment thread src/bunit.core/ComponentParameterCollectionBuilder.cs Outdated
Comment thread src/bunit.core/ComponentParameterCollectionBuilder.cs Outdated
Comment thread src/bunit.core/ComponentParameterCollectionBuilder.cs Outdated
Comment thread src/bunit.core/ComponentParameterCollectionBuilder.cs
Co-authored-by: Egil Hansen <egil@assimilated.dk>
@linkdotnet
Copy link
Copy Markdown
Collaborator Author

I added a very verbose way for the user in terms of the inline-documentation plus the exception. Let me know if the direction fits for you.

@linkdotnet linkdotnet force-pushed the feature/bind branch 2 times, most recently from 4ecee0c to 0494d35 Compare June 13, 2022 14:54
Comment thread docs/samples/tests/xunit/TwoWayBindingTest.cs Outdated
Comment thread docs/site/docs/providing-input/passing-parameters-to-components.md Outdated
Comment thread docs/samples/tests/razor/TwoWayBindingTest.razor Outdated
Comment thread docs/site/docs/providing-input/passing-parameters-to-components.md Outdated
Comment thread src/bunit.core/ComponentParameterCollectionBuilder.cs Outdated
@egil
Copy link
Copy Markdown
Member

egil commented Jun 14, 2022

pushed a few suggestions. let me know what you think

Comment thread src/bunit.core/ComponentParameterCollectionBuilder.cs
Comment thread tests/bunit.core.tests/ComponentParameterCollectionBuilderTests.cs
@linkdotnet
Copy link
Copy Markdown
Collaborator Author

linkdotnet commented Jun 14, 2022

pushed a few suggestions. let me know what you think

Thanks for the better documentation. Seeing the code right now I am still unsure whether or not a chapter in the documentation is more helpful to a certain degree.

Let's assume a user uses Bind inside a EditForm where the ValueExpression is mandatory. Now we give him the opportunity to not pass anything to it, which will lead to a default on our side ("binding" it to initalValue). In the real world this should be bound to the property in the Model-DTO, which is not necessarily the initialValue the user passes in.
I am not 100% sure if this can lead to extremely hard debug able tests.
Are users aware of that fact in the first place?

Guess I'll add some "e2e" tests

@linkdotnet
Copy link
Copy Markdown
Collaborator Author

Guess I'll add some "e2e" tests

After rethinking that, that should not be a problem as initially thought. The only thing is when someone derived from EditForm and implements its own logic, which is highly unliekly.

Anyway I pushed a small change which replaces string.Replace with something like a TrimEnd.
If we have something like that:

public class MyComponent : ComponentBase
{
    [Parameter] public DateTime LastChanged { get; set; }
    [Parameter] public EventCallback<DateTime> LastChangedChanged { get; set; }
}

And the user binds to the wrong property: p => p.Bind(v => v.LastChangedChanged, ...) the helptext in the exception should not tell v => v.Last but v => v.LastChanged. Hope that makes sense.
Do you normally test the message of an exception?

@egil
Copy link
Copy Markdown
Member

egil commented Jun 14, 2022

Guess I'll add some "e2e" tests

After rethinking that, that should not be a problem as initially thought. The only thing is when someone derived from EditForm and implements its own logic, which is highly unliekly.

It's always good to have a few "e2e" tests that uses the Bind method against a real component and checks that we have the expected values in the component.

Perhaps compared to how the @bind directive does it, e.g. we should see the same values passed into the component under test.

Basically render the same component twice using both methods, and then compare the properties to each other. They should be the same.

In a .razor test file, this can be done from the same test.

Anyway I pushed a small change which replaces string.Replace with something like a TrimEnd.
If we have something like that:

public class MyComponent : ComponentBase
{
    [Parameter] public DateTime LastChanged { get; set; }
    [Parameter] public EventCallback<DateTime> LastChangedChanged { get; set; }
}

And the user binds to the wrong property: p => p.Bind(v => v.LastChangedChanged, ...) the helptext in the exception should not tell v => v.Last but v => v.LastChanged. Hope that makes sense.
Do you normally test the message of an exception?

I don't normally test the content of the error message, but do inspect it manually. It seems too unstable. But in certain cases a "contains" check makes sense.

Co-authored-by: Egil Hansen <egil@assimilated.dk>
@linkdotnet
Copy link
Copy Markdown
Collaborator Author

I added an e2e test. The original idea was to compare against ctx.Render(@<MyComponent @bind-Value=".." but that doesn't work for the reasons given in #761.

@linkdotnet
Copy link
Copy Markdown
Collaborator Author

I added an e2e test which tests the expected behavior from an user point of view. If you see open points, let me know

Comment thread CHANGELOG.md Outdated
@egil
Copy link
Copy Markdown
Member

egil commented Jun 23, 2022

I pushed some changes, but dont have time to make the test pass.

The bunit.web.testcomponents project and related test project is deprecated and just kept around for those users who still has a dependency on it, but we should not add new stuff to it.

However, there is nothing wrong with chaning bunit.core.test and bunit.web.test into razor project types so we can write tests in .razor files in them. Thats what I changed in the latest commit.

@linkdotnet
Copy link
Copy Markdown
Collaborator Author

I pushed some changes, but dont have time to make the test pass.

The bunit.web.testcomponents project and related test project is deprecated and just kept around for those users who still has a dependency on it, but we should not add new stuff to it.

However, there is nothing wrong with chaning bunit.core.test and bunit.web.test into razor project types so we can write tests in .razor files in them. Thats what I changed in the latest commit.

Okay. Will check that later including the CHANGELOG.md thingy. Unfortunately not so much time right now

@egil
Copy link
Copy Markdown
Member

egil commented Jun 23, 2022

Going on an extended weekend thing, but will hopefully have time tomorrow. But yeah, we're not in a rush with this, so better get it right than fast.

@linkdotnet
Copy link
Copy Markdown
Collaborator Author

I pushed some changes, but dont have time to make the test pass.

The bunit.web.testcomponents project and related test project is deprecated and just kept around for those users who still has a dependency on it, but we should not add new stuff to it.

However, there is nothing wrong with chaning bunit.core.test and bunit.web.test into razor project types so we can write tests in .razor files in them. Thats what I changed in the latest commit.

Seems like you fixed it. Nice one ;)

Comment thread CHANGELOG.md Outdated
@egil
Copy link
Copy Markdown
Member

egil commented Jun 24, 2022

I think this is done. What do you think?

(this should be a squash merge)

@linkdotnet
Copy link
Copy Markdown
Collaborator Author

I think this is done. What do you think?

(this should be a squash merge)

Looks good to me. Thanks for updating

@egil egil merged commit a923fa2 into main Jun 24, 2022
@egil egil deleted the feature/bind branch June 24, 2022 12:00
egil added a commit that referenced this pull request Jun 27, 2022
* feat: Add bind function to simulate @Bind directive

* add: Documentation and samples

* added null checks and tests

* Apply suggestions from code review

Co-authored-by: Egil Hansen <egil@assimilated.dk>

* Better explanation if Bind encounters an error

* Added Tests

* tweaks to error messages

* Only replace last instance instead of every

* Apply suggestions from code review

Co-authored-by: Egil Hansen <egil@assimilated.dk>

* added e2e test

* wip - compare bind with Bind

* Added small example for bind in CHANGELOG.md

* improved bind compare test

* Update CHANGELOG.md

* fix changelog

Co-authored-by: Egil Hansen <egil@assimilated.dk>
egil added a commit that referenced this pull request Jul 5, 2022
* feat: Add bind function to simulate @Bind directive

* add: Documentation and samples

* added null checks and tests

* Apply suggestions from code review

Co-authored-by: Egil Hansen <egil@assimilated.dk>

* Better explanation if Bind encounters an error

* Added Tests

* tweaks to error messages

* Only replace last instance instead of every

* Apply suggestions from code review

Co-authored-by: Egil Hansen <egil@assimilated.dk>

* added e2e test

* wip - compare bind with Bind

* Added small example for bind in CHANGELOG.md

* improved bind compare test

* Update CHANGELOG.md

* fix changelog

Co-authored-by: Egil Hansen <egil@assimilated.dk>
egil added a commit that referenced this pull request Jul 8, 2022
* feat: Add bind function to simulate @Bind directive

* add: Documentation and samples

* added null checks and tests

* Apply suggestions from code review

Co-authored-by: Egil Hansen <egil@assimilated.dk>

* Better explanation if Bind encounters an error

* Added Tests

* tweaks to error messages

* Only replace last instance instead of every

* Apply suggestions from code review

Co-authored-by: Egil Hansen <egil@assimilated.dk>

* added e2e test

* wip - compare bind with Bind

* Added small example for bind in CHANGELOG.md

* improved bind compare test

* Update CHANGELOG.md

* fix changelog

Co-authored-by: Egil Hansen <egil@assimilated.dk>
linkdotnet added a commit that referenced this pull request Nov 16, 2022
* feat: Add bind function to simulate @Bind directive

* add: Documentation and samples

* added null checks and tests

* Apply suggestions from code review

Co-authored-by: Egil Hansen <egil@assimilated.dk>

* Better explanation if Bind encounters an error

* Added Tests

* tweaks to error messages

* Only replace last instance instead of every

* Apply suggestions from code review

Co-authored-by: Egil Hansen <egil@assimilated.dk>

* added e2e test

* wip - compare bind with Bind

* Added small example for bind in CHANGELOG.md

* improved bind compare test

* Update CHANGELOG.md

* fix changelog

Co-authored-by: Egil Hansen <egil@assimilated.dk>
egil added a commit that referenced this pull request Dec 15, 2022
* feat: Add bind function to simulate @Bind directive

* add: Documentation and samples

* added null checks and tests

* Apply suggestions from code review

Co-authored-by: Egil Hansen <egil@assimilated.dk>

* Better explanation if Bind encounters an error

* Added Tests

* tweaks to error messages

* Only replace last instance instead of every

* Apply suggestions from code review

Co-authored-by: Egil Hansen <egil@assimilated.dk>

* added e2e test

* wip - compare bind with Bind

* Added small example for bind in CHANGELOG.md

* improved bind compare test

* Update CHANGELOG.md

* fix changelog

Co-authored-by: Egil Hansen <egil@assimilated.dk>
egil added a commit that referenced this pull request Jun 10, 2023
* feat: Add bind function to simulate @Bind directive

* add: Documentation and samples

* added null checks and tests

* Apply suggestions from code review

Co-authored-by: Egil Hansen <egil@assimilated.dk>

* Better explanation if Bind encounters an error

* Added Tests

* tweaks to error messages

* Only replace last instance instead of every

* Apply suggestions from code review

Co-authored-by: Egil Hansen <egil@assimilated.dk>

* added e2e test

* wip - compare bind with Bind

* Added small example for bind in CHANGELOG.md

* improved bind compare test

* Update CHANGELOG.md

* fix changelog

Co-authored-by: Egil Hansen <egil@assimilated.dk>
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.

Make @bind-value easier to setup in non-razor syntax

2 participants