enonic / lib-react4xp

React for XP: handling and rendering of pre-built React components in Enonic XP
Apache License 2.0
2 stars 2 forks source link

React4xp pageContributions -> single respond method #8

Closed ComLock closed 5 years ago

ComLock commented 5 years ago

Without the shorthand rendering, there are separate rendering functions for body and pageContributions

If one passes request and pageContributions into the React4xp object, a single respond method should be enough?

ComLock commented 5 years ago

In javascript when you pass an object to a function, you are passing a reference.

By default a javascript object is muteable. In other words you can modify the object inside the function. There is even no need to return the object.

Personally I don't like modifying objects inside functions, but it's really powerful.

ref: https://github.com/enonic/starter-react4xp/blob/examples/src/main/resources/site/parts/04-chaining-example/04-chaining-example.es6#L37-L38

So this:

let pageContributions = firstComp.renderHydrationPageContributions();
    pageContributions = secondComp.renderHydrationPageContributions(pageContributions);

Can become:

const pageContributions = {};
firstComp.modifyPageContributions(pageContributions);
secondComp.modifyPageContributions(pageContributions);

Or perhaps something like this:

const pageContributions = {};

const firstComp = new React4xp({pageContributions, ...});
const secondComp = new React4xp({pageContributions, ...});

return {
    body: thymeleaf.render(VIEW, {
        firstComp: firstComp.getModel(), // method
        secondComp: secondComp.model // or instance property
    }),
    pageContributions
}
espen42 commented 5 years ago

Another one for API discussion.

I feel like keeping the separate rendering methods for body and page contributions... I like to keep some flexibility, opportunities for routing or modifying data along the way in a controller in whatever order you need, etc.

At first I thought:

Yeah, I don't think the renderPageContributions functions are completely pure anyway, I guess the pageContributions object is actually mutated anyway. So I like the modify... suggestion.

Or something similar. As long as there are two different pagecontributions methods (one for pure client-side rendering and one for hydration after SSR), it seems too long with modifyPageContributionsForHydration(pageContributions) and modifyPageContributionsForClientRendering(pageContributions)`. And they can be misunderstood: if you for some reason have one reactComp that's rendered for client-side and another that adds hydration, and you chain them like this: wouldn't it seem a bit like the last one overrides the first one?

How about...

const pageContributions = {};
firstComp.addClientRenderPageContributions(pageContributions);
secondComp.addHydrationPageContributions(pageContributions);

...?

Or wrapping them up internally, something like

const pageContributions = {};
firstComp.serverSideRender.addPageContributions(pageContributions);
secondComp.clientSideRender.addPageContributions(pageContributions);

The body-rendering methods could be wrapped the same way.

...but then I noticed:

The body renderer methods follow the same pattern as the pageContribution renderer methods now. Seems like the cleanest way to go to keep them the same.

But the body renderer method takes and returns a string, and strings can't be mutated inside a function like that - it has to add/modify with the pattern:

a = renderSomething(a);
ComLock commented 5 years ago

Yes, strings are not passed by reference, so they cannot be modified. Unless you put the string inside an object.

Which is why I have not given react4xp the response.body, but rather let react4xp return a model to be used by thymeleaf render. Which is how "I" have done all rendering thus far. You have sorta turned rendering on its head, which is a bit confusing.

Does a react4xp component know wether is shall be client-side or hydrated? If so there is no need for separate function names.

const pageContributions = {};
const firstComp = new React4xp({hydrate:false});
const secondComp = new React4xp({hydrate:true});

firstComp.addPageContributions({pageContributions});
secondComp.addPageContributions({pageContributions});

return {
    body: thymeleaf.render(VIEW, {
        firstComp: firstComp.getModel(), // method
        secondComp: secondComp.model // or instance property
    }),
    pageContributions
}
ComLock commented 5 years ago

Or

firstComp.addPageContributions({pageContributions, hydrate:false});
secondComp.addPageContributions({pageContributions, hydrate:true});
espen42 commented 5 years ago

>> Which is why I have not given react4xp the response.body, but rather let react4xp return a model to be used by thymeleaf render. You have sorta turned rendering on its head, which is a bit confusing.

True - if we assume that thymeleaf always has to be part of the rendering flow. But we offer XSLT and Mustache renderers as well, as standalone options to thymeleaf. So I don't think there's any reason for the rendered HTML output from react4xp to always have to pass through thymeleaf?

Offering a standalone react4xp renderer like the "shorthand" functions makes it possible to have a part/page with only react templating in the View. I wouldn't be surprised if this turned out to be the most useful/used variation.

espen42 commented 5 years ago

One thing, though. The goal from the beginning was to let the "shorthand" .render syntax be as close to the render methods from our thymeleaf, XSLT and mustache libs. To keep at least a basic usage level similar to how users have used XP before. The basic signature for all of them is:

render(viewOrTemplateOrWhateverWord, modelOrPropertiesOrWhatchammacallit).

I guess I've drifted away from that goal - in the render syntax in react4xp the way it is now, we have render(request, parameters), where both "viewOrTemplate" and "modelOrProperties" are part of parameters. So, how to get closer and get the react4xp.render API signature better:

So, some suggestions from that

render(entry, props, request, params) since that's closest to the common render syntax?

render(entry, request, props, params) since request is mandatory but props is optional?

render(entry, request, params) where the props are an optional field in params?

espen42 commented 5 years ago

I'll go for this option: render(entry, props, request, params), with a clientRender flag.

sigdestad commented 5 years ago

What specified the jsx template here?

espen42 commented 5 years ago

The entry argument.