Skip to content

Revisit RemoteRenderer deriving from HtmlRenderer #7805

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

Closed
pranavkm opened this issue Feb 21, 2019 · 5 comments
Closed

Revisit RemoteRenderer deriving from HtmlRenderer #7805

pranavkm opened this issue Feb 21, 2019 · 5 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components Needs: Design This issue requires design work before implementating.
Milestone

Comments

@pranavkm
Copy link
Contributor

Feels like we may need to think this through.

@pranavkm pranavkm added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Feb 21, 2019
@pranavkm pranavkm added this to the 3.0.0-preview4 milestone Feb 21, 2019
@pranavkm pranavkm added the area-blazor Includes: Blazor, Razor Components label Feb 21, 2019
@javiercn
Copy link
Member

Can you elaborate?

@SteveSandersonMS
Copy link
Member

I'd be very happy for us to revisit the arrangement of our Renderer family tree altogether! From a comment elsewhere:

I'm becoming quite concerned about the state of Renderer.cs. Not exclusively because of this PR, but because of a series of changes that have stacked up over time. Apart from just being a long file, it has so many intricate behaviors (some managed based on internal mutable state), and lots of assumptions about what things happen synchronously versus asynchronously. There's so much coupling between the inner workings of the Renderer base class and its subclasses, especially when making a bunch more stuff protected in this PR.

Although it's possible that it's currently bug-free in all our subclasses, it's getting really hard to reason about, and I think eventually this is going to burn us. [...] we need to start considering extensions to the Renderer base class as problematic and possibly needing a broader refactor first.

I'd really like to see a simpler base class for the core-most logic. Perhaps this could tie in with restructuring where "remote" and "prerendering" concerns live.

@javiercn
Copy link
Member

@SteveSandersonMS I think I agree with you.
IMO we could break the functionality into 3 different pieces.
ComponentStateManager -> Handles the component state trees
RenderQueue -> Handles the rendering process and when to update the display.
Renderer -> Sits at the top and coordinates the two of them. Provides a couple of protected virtual methods:

  • CreateRenderQueue
  • CreateStateManager

That calls to initialize them.

  • Renderer dispatches most of the functionality to the state manager and the RenderQueue

@SteveSandersonMS
Copy link
Member

Maybe! I'd be interested in trying to break down why the current code structure is inadequate, what concerns are intermingled, and what would have to be true about different designs to make things clearer while handling all our use cases.

@mkArtakMSFT mkArtakMSFT modified the milestones: 3.0.0-preview6, Backlog Apr 19, 2019
@mkArtakMSFT mkArtakMSFT removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 9, 2019
@pranavkm
Copy link
Contributor Author

Looks like this was addressed as part of #12996. Closing since the original intent of this issue is resolved.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components Needs: Design This issue requires design work before implementating.
Projects
None yet
Development

No branches or pull requests

4 participants