jedireza / hapi-react-views

:package: A hapi view engine for React components
MIT License
231 stars 33 forks source link

Support layout views #14

Closed jevakallio closed 9 years ago

jevakallio commented 9 years ago

Currently hapi-react-views doesn't fully support using server.views.layout templates (docs).

The problems that arise are:

This pull request contains fixes for both above issues, as well as tests and updated documentation. The tests and associated fixtures should hopefully help understand the indended use case.

@jedireza please let me know if this PR needs any improvement.

jedireza commented 9 years ago

Thanks for opening a pull request. I'll be reviewing this soon.

jedireza commented 9 years ago

Did you get a chance to see my PR to your fork with some modifications? https://github.com/jevakallio/hapi-react-views/pull/1

jedireza commented 9 years ago

In Aqua, I didn't need template views because it seemed more semantic to compose React views. For example:

https://github.com/jedireza/aqua/blob/master/server/web/about/index.jsx

I'd like to continue discussing the pros and cons of these approaches.

jevakallio commented 9 years ago

I agree that the approach I proposed is a bit dirty under the covers (the string replacement and setDangerouslyInnerHTML in particular).

The solution in the Aqua application assumes that the entire HTML document is a React application. This is fine, but especially in cases where rewriting a formerly client-only React application to an isomorphic one, React typically only handles the document body, or some element within it.

My PR basically just supports using JSX rendering to create a basic, non React-managed HTML layout where a React application can be mounted. The reason to use it was to avoid bringing on a secondary templating engine for the layout.

Ultimately react-hapi-views is your project and you can feel free to merge or reject this feature. I was just experimenting with your library and saw what I felt was a valuable feature missing.

abritinthebay commented 9 years ago

Honestly? Not a fan of the implementation here. However it's hard to do (and this is exactly why Hapi discourages using the way it currently implements layouts) due to Hapi's implementation not telling the engine when it is passed a layout.

I ended up doing something like this, and it works great:

       reply.view('layouts/mylayout', {
            title: "My Page Title",
            content: Views.myContentView,
            contentProps: { some: "prop"}
        });

Then in the layout just using <this.props.content {...this.props.contentProps} /> as the content hook. Works cleanly and it isn't really any more verbose than the layout solution.

jedireza commented 9 years ago

@abritinthebay thanks for sharing that. And in case you didn't see it from above:

In Aqua, I didn't need template views because it seemed more semantic to compose React views. For example:

https://github.com/jedireza/aqua/blob/master/server/web/about/index.jsx#L2

I like this option. It feels more composable and React-y.

abritinthebay commented 9 years ago

Yeah, it's the same basic idea. In the code above I'm just composing via the view command and nesting rather than composing inside the View itself. That means that the view doesn't always have to be a top-level component - greater flexibility but the same effect.

As you hadn't closed/rejected this I figured you were still keeping it open as an option. :)

jedireza commented 9 years ago

As you hadn't closed/rejected this I figured you were still keeping it open as an option. :)

Yeah sort of. There hasn't been much discussion about it. One thing that I don't like is that it must include a pattern of using dangerouslySetInnerHTML in the module itself. Using that inherently has security implications. And I'd rather avoid promoting a pattern that may be less secure.

jedireza commented 9 years ago

Closing in favor of component style layouts.

There is also an example of this in the README now: https://github.com/jedireza/hapi-react-views/#rendering-with-layouts