Modified EditForm to return _fixedEditContext via the EditContext parameter #24007

chrissainty · 2020-07-16T13:49:13Z

Modified the EditContext parameter to return _fixedEditContext
Introduced a new field, _providedEditContext which stores any EditContext provided by the developer. This means we don't break existing functionality.
Updates to OnParametersSet to use the new _providedEditContext field instead of the EditContext property
Unit tests to cover new EditForm functionality

Addresses #12238
Addresses #23354

…ameter. Also added some tests to cover the new functionality

javiercn · 2020-07-16T14:04:02Z

src/Components/Web/src/Forms/EditForm.cs

        private readonly Func<Task> _handleSubmitDelegate; // Cache to avoid per-render allocations

        private EditContext? _fixedEditContext;
+        private EditContext? _providedEditContext;


I'm not super familiar with this area, do we need the extra field? Or can we make _providedContext hold the right value whether it received a Model or a Context

The _providedEditContext field is there to track if the EditContext was provided directly via the EditContext parameter. This mean we don't break the current functionality in the OnParametersSet method which checks if the developer has provided a value for both EditContext and Model.

javiercn · 2020-07-16T14:04:19Z

src/Components/Web/test/Forms/EditFormTest.cs

+
+namespace Microsoft.AspNetCore.Components.Forms
+{
+    public class EditFormTest


Thanks! I hope the tests are ok, its my first go at writing them.

These tests are looking great. Good stuff!

SteveSandersonMS · 2020-07-16T14:51:37Z

src/Components/Web/src/Forms/EditForm.cs

+        public EditContext? EditContext
+        {
+            get => _fixedEditContext;
+            set => _providedEditContext = value;
+        }


I think this risks being a little weird, because I know we don't recommend people write to parameter properties directly, but if someone does this will be kind of bizarre. If you write a value to this property then try to read it back, you'll get a totally different value :)

Would it be possible instead to do something like:

bool _hasSetEditContextExplicitly; [Parameter] public EditContext? EditContext { get => _fixedEditContext; set { _fixedEditContext = value; _hasSetEditContextExplicitly = true; } }

... and then inside OnParametersSet:

if (_hasSetEditContextExplicitly && Model != null) { throw "Don't set both" } else if (EditContext == null && Model == null) { throw "Set at least one" }

Sorry for not noticing this earlier on the original proposal!

This is why I can't be left alone to review things after lunch 😄

This makes sense, I'll make the change

SteveSandersonMS · 2020-07-16T14:54:17Z

src/Components/Web/test/Forms/EditFormTest.cs

+            var returnedEditContext = editFormComponent.EditContext;
+
+            // Assert
+            Assert.NotNull(returnedEditContext);


Suggested change

Assert.NotNull(returnedEditContext);

Assert.Same(editContext, returnedEditContext);

src/Components/Web/test/Forms/EditFormTest.cs

javiercn · 2020-07-16T14:59:21Z

This is a patch with the changes in the ref assembly, it's not an API change, but the tool is complaining, so we just need to apply that to keep the build happy
Update-ref-assemblies.txt

chrissainty · 2020-07-16T15:56:15Z

@javiercn Is that patch something I need to do? If so what do I need to do? :)

chrissainty · 2020-07-16T15:59:17Z

src/Components/Web/src/Forms/EditForm.cs

+            get => _fixedEditContext;
+            set
+            {
+                if (value != null)


I'm not sure if this was the right thing to do now I'm looking at it here. This would mean that once an EditContext has been set it can't be set back to null. Is that a scenario that should be catered for?

@SteveSandersonMS @javiercn

It's never going to be, is it? even if you set a model, wouldn't that be the case?

I can't think of a reasonable scenario where you would pass in an EditContext and then later decide to set it to null. But just wanted to check

I have the same feeling, but @SteveSandersonMS is the expert on this area, so I would defer the final say to him

If you want a null check, could you throw ArgumentNullException? I think we shouldn't ignore the null value. Either accept it, or throw :)

(Of these options, I'd prefer throwing, since there's no real value in letting people set it back to null)

I added this check so that _hasSetEditContextExplicity doesn't get set to true when the value is null. Perhaps I should adjust the code to this:

public EditContext? EditContext { get => _fixedEditContext; set { _fixedEditContext = value; _hasSetEditContextExplicity = value != null ? true : false; } }

Looks like that would work to me.

Ok I've push the update

javiercn · 2020-07-16T15:59:26Z

@chrissainty you can apply the patch manually (just download it and apply the changes) if you know how, or if you do it the "manual way" inspect the patch, open the file and replace the line manually.

This is part of how we generate reference assemblies for compilation and sometimes it causes this issues. We are working on making this happen automagically I believe

SteveSandersonMS · 2020-07-16T18:56:53Z

src/Components/Web/src/Forms/EditForm.cs

                throw new InvalidOperationException($"{nameof(EditForm)} requires a {nameof(Model)} " +
                    $"parameter, or an {nameof(EditContext)} parameter, but not both.");
            }
+            else if (EditContext == null && Model == null)


Should this now be else if (!_hasSetEditContextExplicitly && Model == null) ?

Because the rule is: you must supply a model if and only if you didn't supply an edit context.

In the event where you:

First supplied a non-null model

Then on the next render supplied a null model

Then we'd want to throw I think, but the existing logic wouldn't.

Yes, agreed. I'll update it.

SteveSandersonMS · 2020-07-16T19:01:30Z

src/Components/Web/src/Forms/EditForm.cs

            // Update _fixedEditContext if we don't have one yet, or if they are supplying a
            // potentially new EditContext, or if they are supplying a different Model
-            if (_fixedEditContext == null || EditContext != null || Model != _fixedEditContext.Model)
+            if (_fixedEditContext == null || (!_hasSetEditContextExplicitly && Model != _fixedEditContext.Model))


Nitpick: I think this could be simplified to:

Suggested change

if (_fixedEditContext == null || (!_hasSetEditContextExplicitly && Model != _fixedEditContext.Model))

if (!_hasSetEditContextExplicitly && Model != _fixedEditContext?.Model)

... because if _hasSetEditContextExplicitly then we know _fixedEditContext != null.

Also the comment above it is slightly out of date now

Strictly speaking the check could be expressed as:

if (Model != null && Model != _fixedEditContext?.Model)

(because we know that if Model != null, then _hasSetEditContextExplicitly is false otherwise we would already have thrown above)

Arguably this is a more natural phrasing because it's just stating that if you give us any new model, we want to use it.

SteveSandersonMS · 2020-07-16T19:06:48Z

Thanks very much for the updates, @chrissainty! I think this is really close to being done now.

One final thought: the name _fixedEditContext no longer really makes sense, and we could simplify it just to being called _editContext now. It's simply the backing field for the EditContext property now. If you feel like making this rename then great, but if not that's absolutely fine and we won't hold up the PR on it :)

chrissainty · 2020-07-16T19:11:53Z

Give me 2 minutes @SteveSandersonMS, I'll get it done! :)

SteveSandersonMS · 2020-07-16T19:28:00Z

Awesome, thanks!

@captainsafia @pranavkm Can I leave it with one of you to merge when the tests pass and if you're happy with it? Up to you whether you also want to ask for/wait for this update too.

chrissainty · 2020-07-16T19:32:58Z

@SteveSandersonMS @captainsafia @pranavkm I've addressed the last comment, all should be good now :)

captainsafia · 2020-07-16T20:07:09Z

@SteveSandersonMS @captainsafia @pranavkm I've addressed the last comment, all should be good now :)

Thanks, Chris! Taking a look now.

captainsafia

Looks like all the feedback has ben addressed here. Thanks for adding the tests!

SteveSandersonMS

Brilliant. Thanks @chrissainty!

chrissainty · 2020-07-16T21:33:37Z

My pleasure! This first sprint has been awesome, I hope we can do it again, soon 😀

captainsafia · 2020-07-17T02:06:55Z

@chrissainty Thank you for the contribution! Your pull request has been merged!

Modified EditForm to return _fixedEditContext via the EditContext par…

08ac708

…ameter. Also added some tests to cover the new functionality

chrissainty requested review from a team and SteveSandersonMS as code owners July 16, 2020 13:49

ghost added the area-blazor Includes: Blazor, Razor Components label Jul 16, 2020

javiercn reviewed Jul 16, 2020

View reviewed changes

javiercn approved these changes Jul 16, 2020

View reviewed changes

SteveSandersonMS reviewed Jul 16, 2020

View reviewed changes

src/Components/Web/test/Forms/EditFormTest.cs Show resolved Hide resolved

pranavkm changed the base branch from master to release/5.0-preview8 July 16, 2020 15:47

pranavkm requested a review from dougbu as a code owner July 16, 2020 15:47

pranavkm changed the base branch from release/5.0-preview8 to master July 16, 2020 15:47

Swapped to boolean to track provided EditContext

903b462

chrissainty commented Jul 16, 2020

View reviewed changes

Chris Sainty added 2 commits July 16, 2020 17:10

Patched ref assembly

89eb7be

Simplified setting _hasSetEditContextExplicitly

e982ced

SteveSandersonMS reviewed Jul 16, 2020

View reviewed changes

Chris Sainty added 2 commits July 16, 2020 20:14

Renamed _fixedEditContext to _editContext

1a91a1c

Updated null check in OnParametersSet

71c1ca2

Simplified check for EditContext updates based on Model changes

fd2fce6

captainsafia approved these changes Jul 16, 2020

View reviewed changes

chrissainty mentioned this pull request Jul 16, 2020

Misleading error message [Blazor] #23354

Closed

SteveSandersonMS approved these changes Jul 16, 2020

View reviewed changes

mkArtakMSFT added the community-contribution Indicates that the PR has been added by a community member label Jul 16, 2020

captainsafia merged commit 7132792 into dotnet:master Jul 17, 2020

captainsafia added this to the 5.0.0-rc1 milestone Jul 17, 2020

captainsafia mentioned this pull request Aug 24, 2020

Validate form/event-related changes in Blazor #25200

Closed

	Assert.NotNull(returnedEditContext);
	Assert.Same(editContext, returnedEditContext);

	if (_fixedEditContext == null \|\| (!_hasSetEditContextExplicitly && Model != _fixedEditContext.Model))
	if (!_hasSetEditContextExplicitly && Model != _fixedEditContext?.Model)

Modified EditForm to return _fixedEditContext via the EditContext parameter #24007

Modified EditForm to return _fixedEditContext via the EditContext parameter #24007

Uh oh!

Conversation

chrissainty commented Jul 16, 2020 • edited by pranavkm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrissainty Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

javiercn commented Jul 16, 2020

Uh oh!

chrissainty commented Jul 16, 2020

Uh oh!

chrissainty Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiercn commented Jul 16, 2020

Uh oh!

SteveSandersonMS Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS commented Jul 16, 2020

Uh oh!

chrissainty commented Jul 16, 2020

Uh oh!

SteveSandersonMS commented Jul 16, 2020

Uh oh!

chrissainty commented Jul 16, 2020

Uh oh!

captainsafia commented Jul 16, 2020

Uh oh!

captainsafia left a comment

Choose a reason for hiding this comment

chrissainty commented Jul 16, 2020 •

edited by pranavkm

Loading

chrissainty Jul 16, 2020 •

edited

Loading

chrissainty Jul 16, 2020 •

edited

Loading

SteveSandersonMS Jul 16, 2020 •

edited

Loading

SteveSandersonMS Jul 16, 2020 •

edited

Loading