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

Potential improvements for overwritten params #20244

Closed
guardrex opened this issue Oct 20, 2020 · 9 comments · Fixed by #20452
Closed

Potential improvements for overwritten params #20244

guardrex opened this issue Oct 20, 2020 · 9 comments · Fixed by #20452
Assignees
Labels
Blazor doc-enhancement Pri2 Priority 2 Source - Docs.ms Docs Customer feedback via GitHub Issue
Projects

Comments

@guardrex
Copy link
Collaborator

cc: @taori ... I'll try get to this as soon as I can. I hope it doesn't take all the way to next year, but we have the .NET 5 GA coming up and then the holidays 🎁⛄ will hit. It could take a week ... or three months! 🙈 ... just depends on the workload.

Thanks for commenting on the doc and providing that cross-link.


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

@dotnet-bot dotnet-bot added ⌚ Not Triaged Blazor Source - Docs.ms Docs Customer feedback via GitHub Issue labels Oct 20, 2020
@guardrex guardrex self-assigned this Oct 20, 2020
@guardrex guardrex added this to Triage in Blazor.Docs via automation Oct 20, 2020
@guardrex guardrex moved this from Triage to P2 - Medium Priority in Blazor.Docs Oct 20, 2020
@simonziegler
Copy link

simonziegler commented Oct 24, 2020

Some follow-up commentary to #24599. We have been building Material.Blazor and really appreciate @SteveSandersonMS's careful explanation in that thread. We have followed his advice and are now using a two-way binding pattern described here and implemented here. This is in fact developed from a port of Steve's RazorComponents.MaterialDesign.

There's one observation we'd like to leave you with. To make this work we have to cope with the fact that when a component is held inside a cascading value, there is effectively a bounce on incoming values. We have to be aware of this (we discovered it by chance and then Steve explained it in detail), and code around it. Before we were aware, we found that some components went into unending oscillation when values changed. We'd therefore urge the ASP.NET team to address this if possible in .NET 6. What we encounted in Material.Blazor is likely to be the same for anybody attempting to work with established JS based component platforms, and we suspect that this will include the burgeoning world of web components.

@guardrex
Copy link
Collaborator Author

@simonziegler Only docs are handled on this repo; so for your feedback to reach the product unit, it's best to open it on their working repo at ...

https://github.com/dotnet/aspnetcore/issues

@simonziegler
Copy link

Oh I see. Thank you! I'll have to raise a new issue there because the stopped further comments from being accessible. However I think this might be worthy of documentation too. It's a bit of a deep issue that will bite programmers.

@guardrex
Copy link
Collaborator Author

guardrex commented Oct 24, 2020

We'd need to see what they say about it before we can document anything. When you open the new issue:

  • Post the cross-link to it here so that I can look at the discussion when I reach this issue.
  • Add a cc: @guardrex to the first comment of the engineering issue so that I can follow the discussion.

@guardrex
Copy link
Collaborator Author

guardrex commented Nov 2, 2020

@captainsafia This is one that we can kick for touch 🏉 until after 5.0 GA. If that seems wise given the workload right now, let's do that. I'll ping u back later.

I was just looking at what Steve wrote at dotnet/aspnetcore#24599 (comment). Although a good start, it's still too complicated in that form. There are are few remarks in it that I can't address myself without more information and a better understanding ...

  • The concept of "side effects" isn't adequately scoped in the passage. It's only defined loosely as doing something that triggers a parent rerender. Are there other side effects than that? It seems so from the general way in which that term is used, but I don't know what they are.
  • The overwritten parameters scenario seems to perhaps occur due to multiple (different) parent rendering conditions. For example, Steve says, "... and in your case ...", which implies that there is at least one other scenario based on the parent component providing the value that results in the unexpected overwriting behavior. Again, it's a bit cryptic on what those other (or the other) scenario is outside of the CascadingValue supplying the parameter.
  • The source of parameter values is weakly described in a few spots (e.g., "whatever previous set of parameters it received"). It's best in a case like that to tell the reader what the source of the change was. Does it just plainly mean the previous value set by the parent or does it include a previous value set by the developer when they directly changed the parameter's value in their code?

Anyway ... my point is just that this is definitely headed in the right direction on explaining why the framework's built-in behavior is interacting poorly with the developer setting the parameters directly, but it's not quite clear enough in its present form for me to shape it into a few doc remarks at the end of the Overwritten parameters section.

We can proceed to try and write something good for this ... for the advanced devs out there that would like to know exactly what's going on here, or we could make a smaller change and move on.

One thing that I would like to do regardless of adding a more in-depth explanation to the end is to touch the beginning of this section. I'm leaning toward an intro to this section along these lines ...

## Overwritten parameters

The Blazor framework generally imposes safe parent-to-child parameter assignment. The goals are to pass parameters between components with the following outcomes:

  • Parameters aren't overwritten unexpectedly.
  • Side-effects are minimized. For example, additional renders are avoided because they may create infinite rendering loops.

[Then, pick up with the current content, which currently starts .......]

New parameter values are supplied, typically overwriting existing ones, when the parent component rerenders.

Consider the following Expander component that:

... the first example ...

btw -- I think we should keep the examples. I think they serve us well. They ...

  • Are cut-'n-paste for a test app so that the dev can experiment with the code and witness the behaviors under discussion.
  • Clearly demonstrate the wrong (writing to the parameter) and right (writing to a field) approaches SxS.

However, we might decide to add something: This whole section is really focused on direct param assignment in C# code but not property setter assignment, which can have similar outcomes. I kind'a feel that we're missing at a minimum a remark about that or at most an actual example of what not to do when using a property setter in this context. Anywho ... that's a secondary thing that we might want to do something about.

If you're interested, would you like to take a hand at floating similar text that's even plainer than Steve's remarks? If not, then I propose that we either just touch the beginning and leave the rest alone or present this back to Steve after 5.0 GA 🏃 (perhaps after he's had a nice long vacation 😅🏖️, too!) to see if he'll clarify it further ..... clarify it so that a small-brained green dinosaur can understand it. 🐲:smile:

@captainsafia
Copy link
Member

I'll let Steve answer the questions on his referenced comment.

but it's not quite clear enough in its present form for me to shape it into a few doc remarks at the end of the Overwritten parameters section.

IMO, we should document the pattern that users should avoid (a component overriding its own parameter) instead of explaining the technical background for it and the workarounds.

@guardrex
Copy link
Collaborator Author

guardrex commented Nov 3, 2020

Ok ... sounds good. I'll ping him later ... no sooner than post 5.0 GA.

document the pattern that users should avoid

I think we're in good general shape on the basics. Not having more detail on the WHY part may end up troubling the product unit with further issues being opened ... maybe.

What he posted is in the direction of what we'd need if we decide to provide an advanced paragraph on it at the end of that section, but it's just not quite clear enough in its present form imo.

Let's just table this for right now. We're all ⛰️⛏️😅 until at least post 5.0 GA, then I think we all need a 🎁 holiday ⛄. I might hold off on asking about this until January.

@taori
Copy link

taori commented Nov 4, 2020

@guardrex Personally in this particular case i would appreciate it the most if "what not do" is very obvious in a Q&A fashion, which can be brief like "when developing a component which uses databinding in the component itself and from a parent component make sure not to override your parameters blablabla" and add a reference into the technical details. Being able to get the technical reason can be interesting when you have deeper issues and are curious to find out the reason, but when your code is not working for some reason it is best to not be hit with a TLDR wall that very moment. Adding a more technical reason in a collapsible paragraph however would also make it easier for the consumer to understand why you should not override parameter values.

Hope that helps

@guardrex
Copy link
Collaborator Author

guardrex commented Nov 4, 2020

Good idea @taori ... I'll work further on the opening remarks of the section. I'll add a line at the top that provides the instruction on what not to do. That's currently missing, and I didn't include it in my proposed update.

We don't use collapsible paragraphs here ... at least it's very rare if we do. If we add a technical explanation, I'd do it at the end and probably mark it with a heading. If the reader gets the basic idea and doesn't want to read the additional content, they can just skip it and move to the next section.

Another option that we sometimes use is to cross-link to the engineering issue. However in this case, I don't think that would work out very well in the engineering issue's present form. Steve's text is too focused on that one scenario and has these few clarity problems. I might pitch that to him: If he were to edit/update the engineering issue for just a few nits of mine, then we could just have a one line sentence at the bottom of this section ... For more information, see [issue link].

@guardrex guardrex moved this from P2 - Medium Priority to In progress in Blazor.Docs Nov 9, 2020
Blazor.Docs automation moved this from In progress to Done Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blazor doc-enhancement Pri2 Priority 2 Source - Docs.ms Docs Customer feedback via GitHub Issue
Projects
Archived in project
Blazor.Docs
  
Done
Development

Successfully merging a pull request may close this issue.

5 participants