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

Add reactive progressive rendering features to MustacheView #16829

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

dsyer
Copy link
Member

@dsyer dsyer commented May 13, 2019

Based on a naming convention, Model attributes of reactive types can be
progressively rendered. If the media type is SSE then an additional
Lambda is available for prepending data: to output lines.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 13, 2019
@bclozel bclozel self-assigned this May 13, 2019
@bclozel bclozel added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels May 13, 2019
@bclozel bclozel added this to the 2.2.x milestone May 13, 2019

private final Charset charset;

private Flux<String> buffers;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't this be List<String> which at the end in getBuffers becomes Flux.fromIterable(this.buffers).map(...)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think that would work, would it? How would you concatMap another Publisher onto it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see the write(Object) method which handles Publisher values. Still isn't it all collected/aggregated before response#writeAndFlushWith is called? So it could be a List<Object> (either String or Publisher) which can then be handled through a combination of Flux.fromIterable and concatWith.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there’s an advantage to doing it that way, I suppose so. What would be the point - you’d end up having to iterate over the list and do the same at the end anyway, wouldn’t you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For efficiency I guess, accumulating vs composing with a new operator for every write. If there are a lot of writes, this could end up with a lot of unecessary objects and an unnecessarily long pipeline.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn’t reactor optimize that away? Could it?

Copy link
Member Author

@dsyer dsyer May 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a list-based refactoring: 3c4fc78. Is that what you had in mind?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a further simplification rstoyanchev@fc5a2cb.

@philwebb
Copy link
Member

@bclozel I remember some talk about this on the Framework call, is the plan to move the code?

@bclozel
Copy link
Member

bclozel commented May 14, 2019

We did discuss this idea but since all the variants are already in Spring Boot (and there’s no benefit in moving those right now) we chose to let them here for now. In general Framework is implementing a couple of views just for reference implementation but would rather see those in third party projects where they can evolve more (like Thymeleaf).

I’ll review this PR and add more tests, especially for memory leaks.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented May 15, 2019

One comment I have is, the wrapping of Flux/Mono/Publisher attributes with FluxLambda suppresses the default behavior of AbstractView to resolve Mono and Flux to concrete values. I think the streaming should be something you opt into for certain attributes. The default assumption should still be to resolve asynchronous values prior to rendering.

In addition, the way I thought of progressive rendering from spring-projects/spring-framework#19547 is that a basic wireframe of the complete HTML page would render, leaving multiple fragments to be completed with asynchronous data from a Flux blended with markup. What I see here is sequential rendering where multiple Flux-based fragments would be rendered one at a time too.

@dsyer
Copy link
Member Author

dsyer commented May 15, 2019

Maybe you missed the documentation ? User has to opt into progressive rendering with a naming convention on the model attribute. I’m happy to argue or yield on what the convention should be, but that seemed the most straightforward and pragmatic approach to me.

@dsyer
Copy link
Member Author

dsyer commented May 15, 2019

The sequential aspect I leave to you. For me what we have here works for the cases that are supported in thymeleaf (and is easier for users), so I was happy to leave it at that. AFAIK thymeleaf only supports one reactive model element per view, which seems limiting to me. But I didn’t go very deep in what happens when multiple attributes are provided here: it works but was only tested with a single attribute.

I'm not sure, but I was under the impression the only way to get multiple fragments streaming at once, or to embed streaming content inside a completely rendered "wireframe", is to use separate SSE endpoints and some JavaScript on the main view. That's the way the Thymeleaf demo works, and it's the same for this proposed feature with Mustache.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented May 16, 2019

User has to opt into progressive rendering with a naming convention on the model attribute.

Indeed I missed the docs, sorry.

The Spring Framework also has conventions for such attributes when added to the model without a name. It looks like currently it is fluxSomething vs somethingFlux which is a bit too easy to forget which is which. Perhaps all we need is just one prefix, like what Thymeleaf does, just to signal a streaming attribute (e.g. stream:something)? It doesn't matter if it is Flux or Publisher.

For me what we have here works for the cases that are supported in thymeleaf (and is easier for users), so I was happy to leave it at that.

Indeed it's a place to start

I didn’t go very deep in what happens when multiple attributes are provided here: it works but was only tested with a single attribute.

I would expect each Flux/fragment to be rendered sequentially.

I'm not sure, but I was under the impression the only way to get multiple fragments streaming at once, or to embed streaming content inside a completely rendered "wireframe", is to use separate SSE endpoints and some JavaScript on the main view.

IIRC there were multiple options. One option was to render a few HTML segments initially, e.g. <div id="section1">...</div>, <div id="section2">...</div>, etc, and then each fragment streamed from the server is matched to the corresponding section by id via JavaScript, which also implies that multiple Publishers are flatmapped rather than concatenated so each section can emit data fragments as they become available.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented May 16, 2019

One correction. The link above to what Thymeleaf does is actually an example of how dialects can contribute async attributes to be resolved prior to rendering (e.g. Publisher but also Supplier and Function). For the streaming scenario, it's not a naming convention but rather a wrapper class.

@dsyer
Copy link
Member Author

dsyer commented May 16, 2019

Yes, the wrapper really didn’t appeal to me. A model is a model, it shouldn’t need special object types that tie it to the view layer.

@dsyer
Copy link
Member Author

dsyer commented May 17, 2019

I'm not really getting what you mean with the fragments and flat maps. It would mean the template has JavaScript in it, right? Would the fragments be rendered at the bottom of the page into a script? Do you have an example? I'm not sure why that wouldn't work with the current implementation, but it's hard without a concrete example.

@rstoyanchev
Copy link
Contributor

Yes, the wrapper really didn’t appeal to me.

I didn't mean to suggest that. Still the problem remains.

@dsyer
Copy link
Member Author

dsyer commented May 20, 2019

I'm not sure I understand what the problem is. There's already a naming convention somewhere and we should try and match it (not clear why that would be)? The test case you linked to didn't really explain to me either the origin or intent of the convention.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented May 20, 2019

The default naming convention for attributes added to the model is "something[Flux|Mono]" while this view looks for attributes named "[flux|mono]Something". The former are resolved to concrete values, while the latter are not. I am just saying this too easy to mix up.

@dsyer
Copy link
Member Author

dsyer commented May 20, 2019

The former are resolved to concrete values because they don't match a convention, however that convention is too close syntactically to the one used by Spring WebFlux already when adding model attributes with no name. But you are not saying that a naming convention is a bad idea? So what would work? Maybe a name that suggests that the rendering will be different (unresolved.* or something)?

@rstoyanchev
Copy link
Contributor

Or async. or maybe async:.

@dsyer
Copy link
Member Author

dsyer commented May 21, 2019

I added support for both "async." and "async:" in the latest commit.

@rstoyanchev
Copy link
Contributor

Are you planning to pic up these rstoyanchev@fc5a2cb?

@dsyer
Copy link
Member Author

dsyer commented May 21, 2019

I wasn't aware of those changes. Can you send a PR to my branch?

@rstoyanchev
Copy link
Contributor

Done

@philwebb philwebb modified the milestones: 2.x, 3.x Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants