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

Wrapping HTML responses with the shell and layout components #524

Open
i-like-robots opened this issue Jul 31, 2019 · 1 comment
Open

Wrapping HTML responses with the shell and layout components #524

i-like-robots opened this issue Jul 31, 2019 · 1 comment
Labels
Help wanted Extra attention is needed Question Further information is requested Server-side integration

Comments

@i-like-robots
Copy link
Contributor

I'm opening this issue to discuss how we should provide a package capable of neatly integrating the shell and layout components into our existing applications in order to "wrap" their HTML output with the shared page bootstrapping, metadata, core branding, and UI.

As initially discussed in #98 "preset" packages like this can be used to prevent applications implementing lots of divergent custom code.

Our current approach

The v0.1.0 n-ui to Page Kit migration guide instructs developers to add a callback argument to Express's response.render() method in order to capture the HTML output so that it can be used for a second render step. This is not too dissimilar to the the existing Handlebars layout mechanism it replaces.

The approach is "intentionally hacky" in order to expose how the parts fits together and it is relatively simple. However, it means Express arguments need to be passed around in addition to configuration data for the shell and layout components which may be confusing and tricky to mentally parse.

// server/controllers/lib/page-kit-wrapper.js
const React = require('react');
const ReactDOM = require('react-dom/server');
const { Shell } = require('@financial-times/dotcom-ui-shell');
const { Layout } = require('@financial-times/dotcom-ui-layout');

module.exports = ({ response, next, shellProps, layoutProps }) => {
	return (error, html) => {
		if (error) {
			return next(error);
		}

		const document = React.createElement(
			Shell,
			{ ...shellProps },
			React.createElement(Layout, { ...layoutProps, contents: html })
		);

		response.send('<!DOCTYPE html>' + ReactDOM.renderToStaticMarkup(document));
	};
};

// server/controllers/page.js
const pageKitWrapper = require('./lib/page-kit-wrapper.js');

module.exports = (request, response, next) => {
	const templateData = {};
	
	const shellProps = {
		pageTitle: content.title,
	};

	const layoutProps = {
		navigationData: response.locals.navigation,
		headerOptions: { ...response.locals.anon }
	};

	const pageKitOptions = { request, response, next, shellProps, layoutProps };
	
	response.render('video.html', templateData, pageKitWrapper(pageKitOptions));
};
@i-like-robots i-like-robots added Help wanted Extra attention is needed Question Further information is requested Server-side integration labels Jul 31, 2019
@i-like-robots
Copy link
Contributor Author

i-like-robots commented Jul 31, 2019

My initial thoughts on this are that I would like where possible for UI components to not know anything about the request/response objects in order to be generic and reusable with any framework or tool.

IMO Express view engines are not especially useful, they are really responsible for three things:

  1. Find that the requested "view" file exists (the location of which is based upon some Express settings)
  2. Merge app.locals, response.locals, and the template data together
  3. Capture any rendering errors and call the fall-through function (next())

In a JSX world the first feature is not very useful because we can always use Node's built in module require() function. The second feature is a one liner now that we have Object.assign() and the spread operator. The third feature is quite useful but trivial to implement.

Given this I suspect it may be simpler to avoid the view engine paradigm and callbacks altogether and do something more generic along these lines:

// server/controllers/page.js
const handlebars = require('../lib/handlebars');
const pageKitPreset = require('@financial-times/dotcom-ui-preset');

module.exports = (request, response, next) => {
	const templateData = {};

	const shellProps = {
		pageTitle: content.title,
	};

	const layoutProps = {
		navigationData: response.locals.navigation,
		headerOptions: { ...response.locals.anon }
	};

	try {
		const html = handlebars.render('views/page.html', {
			...app.locals,
			...response.locals,
			...templateData
		});

		response.send(pageKitPreset({ shellProps, layoutProps, html }));
	} catch (error) {
		next(error);
	}
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help wanted Extra attention is needed Question Further information is requested Server-side integration
Projects
None yet
Development

No branches or pull requests

1 participant