Add reactive progressive rendering features to MustacheView

[dsyer]

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.

[rstoyanchev]

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

[dsyer]

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

[rstoyanchev]

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.

[dsyer]

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?

[rstoyanchev]

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.

[dsyer]

Doesn’t reactor optimize that away? Could it?

[dsyer]

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

[rstoyanchev]

Here is a further simplification rstoyanchev@fc5a2cb.

[philwebb]

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

[bclozel]

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]

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]

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]

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]

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]

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]

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]

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]

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

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

[dsyer]

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]

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]

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]

Or async. or maybe async:.

[dsyer]

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

[rstoyanchev]

Are you planning to pic up these rstoyanchev@fc5a2cb?

[dsyer]

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

[rstoyanchev]

Done