Skip to content

Refactor HTML for apps #302

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 4, 2016
Merged

Refactor HTML for apps #302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
},
"parser": "babel-eslint",
"rules": {
"object-curly-spacing": 0,
"space-before-function-paren": [2, "never"],
"react/prefer-stateless-function": 0,
"object-curly-spacing": "off",
"space-before-function-paren": ["error", "never"],
"react/prefer-stateless-function": "off",
"react/jsx-indent-props": "off",
"react/jsx-closing-bracket-location": "off"
}
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@
"piping": "0.3.1",
"react": "15.0.2",
"react-cookie": "0.4.5",
"react-helmet": "3.1.0",
"react-redux": "4.4.5",
"react-router": "2.4.0",
"redux": "3.5.2",
Expand Down
87 changes: 87 additions & 0 deletions src/core/containers/ServerHtml.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import React, { Component, PropTypes } from 'react';
import ReactDOM from 'react-dom/server';
import serialize from 'serialize-javascript';
import Helmet from 'react-helmet';


export default class ServerHtml extends Component {

static propTypes = {
appName: PropTypes.string.isRequired,
assets: PropTypes.object.isRequired,
component: PropTypes.object.isRequired,
includeSri: PropTypes.bool,
sriData: PropTypes.object,
store: PropTypes.object.isRequired,
};

getStatic({filePath, type, index}) {
const { includeSri, sriData, appName } = this.props;
const leafName = filePath.split('/').pop();
let sriProps = {};
// Only output files for the current app.
if (leafName.startsWith(appName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like it should be outside of this component to me. Probably isDeployed too. That would mean this helper could likely go away and just the SRI part could be shared:

<ServerHtml styles={filterForApp(appName, assets.style)} includeSri={isDeployed} />

// ...
getStyles() {
  return this.props.styles.map((style) =>
    <link rel="stylesheet" href={style} {...this.sriProps(style)} />);
}

sriProps(filePath) {
  const { includeSri, sriData } = this.props;
  if (!includeSri) {
    return {};
  }
  const sriProps = {
    integrity: sriData[filePath.split('/').pop()],
    crossOrigin: 'anonymous',
  };
  if (!sriProps.integrity) {
    throw new Error(...);
  }
  return sriProps;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of includeSri, I think we should conside moving the sri helper out in a future branch and/or as the need arises. Since we need to try and get back to adding core features ASAP.

if (includeSri) {
sriProps = {
integrity: sriData[leafName],
crossOrigin: 'anonymous',
};
if (!sriProps.integrity) {
throw new Error(`SRI Data is missing for ${leafName}`);
}
}
switch (type) {
case 'css':
return (<link href={filePath} {...sriProps}
key={type + index}
rel="stylesheet" type="text/css" />);
case 'js':
return <script key={type + index} src={filePath} {...sriProps}></script>;
default:
throw new Error('Unknown static type');
}
} else {
return null;
}
}

getStyle() {
const { assets } = this.props;
return Object.keys(assets.styles).map((style, index) =>
this.getStatic({filePath: assets.styles[style], type: 'css', index}));
}

getScript() {
const { assets } = this.props;
return Object.keys(assets.javascript).map((js, index) =>
this.getStatic({filePath: assets.javascript[js], type: 'js', index}));
}

render() {
const { component, store } = this.props;
// This must happen before Helmet.rewind() see
// https://github.com/nfl/react-helmet#server-usage for more info.
const content = component ? ReactDOM.renderToString(component) : '';
const head = Helmet.rewind();
const htmlAttrs = head.htmlAttributes.toComponent();
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the API for helmet pretty disappointing. Shouldn't this be .toObject()? What does it mean to rewind a helmet? I'm no American football expert but I don't think that's a thing.

It does what we need though, so I guess I can put up with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's weird that one - this was the example. https://github.com/nfl/react-helmet#as-react-components

I think I'm going to remove that since we don't need it yet. When we do we can add it and test it properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I'm wrong we're using it for lang attrs. I'll add some test coverage for that piece.


return (
<html {...htmlAttrs}>
<head>
<meta charSet="utf-8" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the uppercase S a react thing? I feel like it should just be charset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the test caught that not working. JSX is full of those kind of weird camel attr names like className instead of classname.

<meta name="viewport" content="width=device-width, initial-scale=1" />
<link rel="shortcut icon" href="/favicon.ico" />
{head.title.toComponent()}
{head.meta.toComponent()}
{this.getStyle()}
</head>
<body>
<div id="react-view" dangerouslySetInnerHTML={{__html: content}} />
<script dangerouslySetInnerHTML={{__html: serialize(store.getState())}}
type="application/json" id="redux-store-state" />
{this.getScript()}
</body>
</html>
);
}
}
108 changes: 41 additions & 67 deletions src/core/server/base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,18 @@ import fs from 'fs';
import Express from 'express';
import helmet from 'helmet';
import path from 'path';
import serialize from 'serialize-javascript';
import cookie from 'react-cookie';
import React from 'react';
import ReactDOM from 'react-dom/server';
import ReactHelmet from 'react-helmet';

import { stripIndent } from 'common-tags';
import { renderToString } from 'react-dom/server';
import { Provider } from 'react-redux';
import { match } from 'react-router';
import { ReduxAsyncConnect, loadOnServer } from 'redux-async-connect';

import WebpackIsomorphicTools from 'webpack-isomorphic-tools';
import WebpackIsomorphicToolsConfig from 'webpack-isomorphic-tools-config';
import ServerHtml from 'core/containers/ServerHtml';

import config from 'config';
import { setJWT } from 'core/actions';
Expand All @@ -29,7 +29,7 @@ global.CLIENT = false;
global.SERVER = true;
global.DEVELOPMENT = env === 'development';

export default function(routes, createStore) {
function baseServer(routes, createStore, { appInstanceName = appName } = {}) {
const app = new Express();
app.disable('x-powered-by');

Expand Down Expand Up @@ -58,7 +58,7 @@ export default function(routes, createStore) {
app.post('/__cspreport__', (req, res) => res.status(200).end('ok'));

// Redirect from / for the search app it's a 302 to prevent caching.
if (appName === 'search') {
if (appInstanceName === 'search') {
app.get('/', (req, res) => res.redirect(302, '/search'));
}

Expand All @@ -82,67 +82,37 @@ export default function(routes, createStore) {
store.dispatch(setJWT(token));
}

return loadOnServer({...renderProps, store}).then(() => {
const InitialComponent = (
<Provider store={store} key="provider">
<ReduxAsyncConnect {...renderProps} />
</Provider>
);

const componentHTML = renderToString(InitialComponent);

const assets = webpackIsomorphicTools.assets();

// Get SRI for deployed services only.
const sri = isDeployed ? JSON.parse(
fs.readFileSync(path.join(config.get('basePath'), 'dist/sri.json'))
) : {};

const styles = Object.keys(assets.styles).map((style) => {
const cssHash = sri[path.basename(assets.styles[style])];
if (isDeployed && !cssHash) {
throw new Error('Missing SRI Data');
}
const cssSRI = sri && cssHash ? ` integrity="${cssHash}" crossorigin="anonymous"` : '';
return `<link href="${assets.styles[style]}"${cssSRI}
rel="stylesheet" type="text/css" />`;
}).join('\n');

const script = Object.keys(assets.javascript).map((js) => {
const jsHash = sri[path.basename(assets.javascript[js])];
if (isDeployed && !jsHash) {
throw new Error('Missing SRI Data');
}
const jsSRI = sri && jsHash ? ` integrity="${jsHash}" crossorigin="anonymous"` : '';
return `<script src="${assets.javascript[js]}"${jsSRI}></script>`;
}).join('\n');

const HTML = stripIndent`
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>Isomorphic Redux Demo</title>
<meta name="viewport" content="width=device-width, initial-scale=1" />
${styles}
</head>
<body>
<div id="react-view">${componentHTML}</div>
<script type="application/json" id="redux-store-state">
${serialize(store.getState())}
</script>
${script}
</body>
</html>`;

res.header('Content-Type', 'text/html');
return res.end(HTML);
})
.catch((error) => {
// eslint-disable-next-line no-console
console.error(error.stack);
res.status(500).end(errorString);
});
return loadOnServer({...renderProps, store})
.then(() => {
const InitialComponent = (
<Provider store={store} key="provider">
<ReduxAsyncConnect {...renderProps} />
</Provider>
);

// Get SRI for deployed services only.
const sriData = (isDeployed) ? JSON.parse(
fs.readFileSync(path.join(config.get('basePath'), 'dist/sri.json'))
) : {};

const pageProps = {
appName: appInstanceName,
assets: webpackIsomorphicTools.assets(),
component: InitialComponent,
head: ReactHelmet.rewind(),
sriData,
includeSri: isDeployed,
store,
};

const HTML = ReactDOM.renderToString(<ServerHtml {...pageProps} />);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the component need to be passed in here or could it be a child instead?

const HTML = ReactDOM.renderToString(
  <ServerHtml {...pageProps}>
    <Provider store={store} key="provider">
      <ReduxAsyncConnect {...renderProps} />
    </Provider>
  </ServerHtml>
);

Then ServerHtml would have <div id="react-view">{this.props.children}</div> instead of <div id="react-view" dangerouslySetInnerHTML={{__html: content}} />.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that seems nice. Let me try that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately we can do that because Helmet.rewind needs to be called on the server after the component is rendered. Using child props caused the data to not be available.

res.send(`<!DOCTYPE html>${HTML}`);
})
.catch((error) => {
// eslint-disable-next-line no-console
console.error(error.stack);
res.status(500).end(errorString);
});
});
});

Expand Down Expand Up @@ -174,7 +144,9 @@ export function runServer({listen = true, app = appName} = {}) {
// Webpack Isomorphic tools is ready
// now fire up the actual server.
return new Promise((resolve, reject) => {
const server = require(`${app}/server`).default;
const routes = require(`${app}/routes`).default;
const createStore = require(`${app}/store`).default;
const server = baseServer(routes, createStore, {appInstanceName: app});
if (listen === true) {
server.listen(port, host, (err) => {
if (err) {
Expand All @@ -196,3 +168,5 @@ export function runServer({listen = true, app = appName} = {}) {
console.error(err);
});
}

export default baseServer;
5 changes: 5 additions & 0 deletions src/disco/containers/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import React, { PropTypes } from 'react';
import Helmet from 'react-helmet';

import 'disco/css/App.scss';
import { gettext as _ } from 'core/utils';


export default class App extends React.Component {
Expand All @@ -12,6 +14,9 @@ export default class App extends React.Component {
const { children } = this.props;
return (
<div className="disco-pane">
<Helmet
defaultTitle={_('Discover Add-ons')}
/>
{children}
</div>
);
Expand Down
7 changes: 0 additions & 7 deletions src/disco/server.js
View file
Open in desktop

This file was deleted.

5 changes: 5 additions & 0 deletions src/search/containers/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import React, { PropTypes } from 'react';
import Helmet from 'react-helmet';

import 'search/css/App.scss';
import { gettext as _ } from 'core/utils';


export default class App extends React.Component {
Expand All @@ -12,6 +14,9 @@ export default class App extends React.Component {
const { children } = this.props;
return (
<div className="search-page">
<Helmet
defaultTitle={_('Add-ons Search')}
/>
{children}
</div>
);
Expand Down
7 changes: 0 additions & 7 deletions src/search/server.js
View file
Open in desktop

This file was deleted.

Loading