Skip to content
This repository was archived by the owner on Feb 25, 2021. It is now read-only.

trying to fix 1009 by changing the AssignToProperties method behavior #1108

Closed
wants to merge 8 commits into from

Conversation

RemiBou
Copy link
Contributor

@RemiBou RemiBou commented Jul 9, 2018

So here is my other attempt on fixing this bug, I'll let the blazor team decide which way they prefer to go

@RemiBou
Copy link
Contributor Author

RemiBou commented Oct 5, 2018

@SteveSandersonMS can you give a quick feedback on this PR ? If you think it needs more work please tell me. The build error doesn't seems to be related to the PR content but I'll dig it if you think I'm onto something.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Oct 10, 2018

Thanks for suggesting this!

The approach of using the cached parameter writers to know what values to blank out is promising. We would probably want to rename the method AssignToProperties to something like SetBindableProperties to clarify its updated behavior.

The code in the PR as-is is pretty allocatey (for example, instantiating a new List during every AssignToProperties call, and using Except which is going to allocate more enumerators), so for performance we'd need to find ways of doing this without the extra heap allocations. I'm happy to look into that when I get to it, or please feel free to look for ways to avoid the per-invocation allocations if you feel able.

Thanks again for contributing this!

@RemiBou
Copy link
Contributor Author

RemiBou commented Oct 10, 2018

Ok I'll have a look at this. i'm not a big expert on avoiding allocations, but I'll try to dig it. Thanks for the feedback

@RemiBou
Copy link
Contributor Author

RemiBou commented Oct 12, 2018

@SteveSandersonMS I changed the code so there is less allocation :

  • got rid of WriteParameterAction (its creation showed a warning in clr heap analyzer that the compiler will emit a new class holding the property setter as a field) an use only IPropertySetter
  • Add the default value in propertySetter
  • use a local copy of the propertywriters list and remove the used one, then browse the remainings to set the default value
  • the only warning I got in Clr Heap Allocation Analyzer seems to be the static fields, the exceptions and the loop line 77 but we don't have much choice here.

And I renamed the method to "SetParameterProperty" because of the attribute name.

What's the point of the generic PropertySetter here ? it's interface is not using generic parameters.

@RemiBou
Copy link
Contributor Author

RemiBou commented Oct 17, 2018

@SteveSandersonMS ok the build is fixed after fixing the conflict with PR #1545

@RemiBou
Copy link
Contributor Author

RemiBou commented Nov 7, 2018

I'll fix the CI error this week, I don't understand how my change causes error on non-Windows E2E test but it's not failing on other PR so ...

@RemiBou
Copy link
Contributor Author

RemiBou commented Nov 9, 2018

@SteveSandersonMS or @rynowak I understand why the test fails here, but I need your opinion how I need to fix it.

The problem is on \samples\ServerSideBlazor.Client\Pages\FetchData.cshtml. On line 53 we set StartDate to DateTime.Now, but if we load /fetchdata we don't send any date in the url, so my patch forces it to DateTime default value (0001/01/01 ...) and then the test fails because "StartDate.AddDays(-5)" throws an ArgumentOutOfRange" error (I think it doesn't fail on mono but does fail on core, that's why only this test is failing).

I can't find any smart way to solve it :

  • if I put a check on ParameterCollectionExtension : parameterValue != default value then my patch is useless
  • if we add StartDate = DateTime.Now after the call to setParameters then the eventual parameter send in the url will always be erased

Maybe we can add a DefaultValue to the parameter attribute but it will or in the fetchData.cshtml set StartDate as nullable and check for null value after setParameters

`
[Parameter] DateTime? StartDate { get; set; }

WeatherForecast[] forecasts;

public override void SetParameters(ParameterCollection parameters)
{

    base.SetParameters(parameters);

if(!StartDate.HasValue)
StartDate = DateTime.Now;
}
`

What do you think ?

@RemiBou
Copy link
Contributor Author

RemiBou commented Nov 25, 2018

Hi all, do I need to keep this PR up-to-date with master, choose myself how to fix the CI (see previous comment), improve the code somehow, or should I close it ?

@SteveSandersonMS
Copy link
Member

Currently we’re focused on the code migration work. Once that’s done we can resume attending to PRs.

In the meantime if you want to simplify migrating this PR you could rebase it on master. Thanks!

@RemiBou
Copy link
Contributor Author

RemiBou commented Nov 25, 2018 via email

@RemiBou
Copy link
Contributor Author

RemiBou commented Nov 25, 2018

Merge is done, the same tests are failing.

FYI the file move was not correctly handled by git, I had to reproduce all the changes on the new files. It might be a problem on bigger PR.

@Andrzej-W
Copy link

@RemiBou thank you very much that after a few months you are still trying to fix this bug. I really appreciate your work (and of course the work of the entire Blazor team).

@SteveSandersonMS
Copy link
Member

Thanks again for this, @RemiBou! It's now moved to dotnet/aspnetcore#4797, where it will be merged shortly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants