Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

Support configuring Html props from react-server #1661

Merged
merged 3 commits into from
Oct 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions packages/react-html/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

<!-- ## [Unreleased] -->
BPScott marked this conversation as resolved.
Show resolved Hide resolved

- Update `HtmlProps` to mark children as optional (same as any React component) and export it ([#1661](https://github.com/Shopify/quilt/pull/1661))
BPScott marked this conversation as resolved.
Show resolved Hide resolved

## [10.0.1] - 2020-10-20

- Updated `tslib` dependency to `^1.14.1`. [#1657](https://github.com/Shopify/quilt/pull/1657)
Expand Down
8 changes: 4 additions & 4 deletions packages/react-html/src/server/components/Html.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ export interface InlineStyle {
content: string;
}

export interface Props {
export interface HtmlProps {
manager?: HtmlManager;
hydrationManager?: HydrationManager;
children: React.ReactElement<any> | string;
children?: React.ReactElement<any> | string;
Copy link
Member Author

Choose a reason for hiding this comment

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

Exporting HtmlProps and marking children as optional means I can reuse this type down in the definition of createRender's options object (https://github.com/Shopify/quilt/pull/1661/files#diff-3db342f047695e4ca4277cd8264ac3b2db110ec15ec6c34ed697ce2859004df0R57) rather than creating a new type that omits children

locale?: string;
styles?: Asset[];
inlineStyles?: InlineStyle[];
Expand All @@ -37,7 +37,7 @@ export interface Props {
export default function Html({
manager,
hydrationManager,
children,
children = '',
locale = 'en',
blockingScripts = [],
scripts = [],
Expand All @@ -46,7 +46,7 @@ export default function Html({
preloadAssets = [],
headMarkup = null,
bodyMarkup = null,
}: Props) {
}: HtmlProps) {
const markup =
typeof children === 'string'
? children
Expand Down
2 changes: 1 addition & 1 deletion packages/react-html/src/server/components/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
export {default as Serialize} from './Serialize';
export {default as Html} from './Html';
export {default as Html, HtmlProps} from './Html';
2 changes: 2 additions & 0 deletions packages/react-server/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

<!-- ## [Unreleased] -->

- Added `htmlProps` to the options for `createRender` and `createServer`, these props will be passed into the call to `@shopify/react-html`'s `<Html>` component ([#1661](https://github.com/Shopify/quilt/pull/1661))

## [0.18.4] - 2020-10-20

- Added `tslib@^1.14.1` in the list of dependencies. [#1657](https://github.com/Shopify/quilt/pull/1657)
Expand Down
3 changes: 3 additions & 0 deletions packages/react-server/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ interface Options {
debug?: boolean;
// a function similar to the render option but specifically used to render error pages for production SSR errors
renderError: RenderFunction;
// additional props to pass into the Html component, or a function that takes a Koa.Context and returns a props object
// See https://github.com/Shopify/quilt/blob/master/packages/react-html/README.md#html-
htmlProps?: HtmlProps | (ctx: Context) => HtmlProps
}
```

Expand Down
2 changes: 1 addition & 1 deletion packages/react-server/src/render/index.ts
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export {createRender, Context, RenderFunction} from './render';
export {createRender, Context, RenderFunction, RenderOptions} from './render';
29 changes: 26 additions & 3 deletions packages/react-server/src/render/render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {Context} from 'koa';
import compose from 'koa-compose';
import {
Html,
HtmlProps,
HtmlManager,
HtmlContext,
stream,
Expand Down Expand Up @@ -46,26 +47,31 @@ interface Data {
value: {[key: string]: any} | undefined;
}

type Options = Pick<
export type RenderOptions = Pick<
NonNullable<ArgumentAtIndex<typeof extract, 1>>,
'afterEachPass' | 'betweenEachPass'
> & {
assetPrefix?: string;
assetName?: string | ValueFromContext<string>;
renderError?: RenderFunction;
htmlProps?: HtmlProps | ValueFromContext<HtmlProps>;
};

/**
* Creates a Koa middleware for rendering an `@shopify/react-html` based React application defined by `render`.
* @param render
* @param options
*/
export function createRender(render: RenderFunction, options: Options = {}) {
export function createRender(
render: RenderFunction,
options: RenderOptions = {},
) {
const manifestPath = getManifestPath(process.cwd());
const {
assetPrefix,
assetName: assetNameInput = 'main',
renderError,
htmlProps: htmlPropsInput = {},
} = options;

async function renderFunction(ctx: Context) {
Expand All @@ -74,6 +80,15 @@ export function createRender(render: RenderFunction, options: Options = {}) {
? assetNameInput(ctx)
: assetNameInput;

const {
scripts: additionalScripts = [],
styles: additionalStyles = [],
...additionalHtmlProps
} =
typeof htmlPropsInput === 'function'
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit nitpicky but I found this a bit difficult to parse at first glance because of the line break.
Maybe split this up into 2 expressions by pulling out this ternary into a const props or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, the linebreak after the = is quite unconventional. Not sure it's worth adding a level of indirection to fix this, though.

? htmlPropsInput(ctx)
: htmlPropsInput;

const logger = getLogger(ctx) || console;
const assets = getAssets(ctx);

Expand Down Expand Up @@ -133,8 +148,16 @@ export function createRender(render: RenderFunction, options: Options = {}) {
assets.scripts({name: assetName, asyncAssets: immediateAsyncAssets}),
]);

styles.push(...additionalStyles);
Copy link
Member Author

Choose a reason for hiding this comment

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

Making sure that if somebody decides to use styles/scripts then they don't clobber the assets provided by getAssets. I've added a test case for this.

scripts.push(...additionalScripts);

const response = stream(
<Html manager={htmlManager} styles={styles} scripts={scripts}>
<Html
{...additionalHtmlProps}
manager={htmlManager}
styles={styles}
scripts={scripts}
>
<Providers>{app}</Providers>
</Html>,
);
Expand Down
57 changes: 55 additions & 2 deletions packages/react-server/src/render/test/render.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import withEnv from '@shopify/with-env';
import {createRender, Context} from '../render';
import {mockMiddleware} from '../../test/utilities';

const mockAssetsScripts = jest.fn(() => Promise.resolve([]));
const mockAssetsStyles = jest.fn(() => Promise.resolve([]));
const mockAssetsScripts = jest.fn(() => Promise.resolve([{path: 'main.js'}]));
const mockAssetsStyles = jest.fn(() => Promise.resolve([{path: 'main.css'}]));

jest.mock('@shopify/sewing-kit-koa', () => ({
middleware: jest.fn(() => mockMiddleware),
Expand Down Expand Up @@ -36,6 +36,59 @@ describe('createRender', () => {
expect(await readStream(ctx.body)).toContain(myCoolApp);
});

it.each([
[
'as Plain object',
{
scripts: [{path: '/extraScript.js'}],
styles: [{path: '/extraStyle.css'}],
headMarkup: <script>let ScriptFromHeadMarkup = 1;</script>,
},
],
[
'as Function',
() => ({
scripts: [{path: '/extraScript.js'}],
styles: [{path: '/extraStyle.css'}],
headMarkup: <script>let ScriptFromHeadMarkup = 1;</script>,
}),
],
])(
'response contains data passed in through htmlProps (%s)',
async (style, htmlProps) => {
const myCoolApp = 'My cool app';
const ctx = createMockContext();

const renderFunction = createRender(() => <>{myCoolApp}</>, {
htmlProps,
});
await renderFunction(ctx, noop);

const bodyResult = await readStream(ctx.body);

// Assets from manifest are still present
expect(bodyResult).toContain(
'<script type="text/javascript" src="main.js" crossorigin="anonymous" defer=""></script>',
);
expect(bodyResult).toContain(
'<link rel="stylesheet" type="text/css" href="main.css" crossorigin="anonymous"/>',
);

// Additional script/style assets are added
expect(bodyResult).toContain(
'<script type="text/javascript" src="/extraScript.js" crossorigin="anonymous" defer=""></script>',
);
expect(bodyResult).toContain(
'<link rel="stylesheet" type="text/css" href="/extraStyle.css" crossorigin="anonymous"/>',
);

// Other props work
expect(bodyResult).toContain(
'<script>let ScriptFromHeadMarkup = 1;</script>',
);
},
);

it('response contains x-quilt-data from headers', async () => {
const myCoolApp = 'My cool app';
const data = {foo: 'bar'};
Expand Down
13 changes: 8 additions & 5 deletions packages/react-server/src/server/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,10 @@ import Koa, {Context} from 'koa';
import compose from 'koa-compose';
import mount from 'koa-mount';

import {createRender, RenderFunction} from '../render';
import {createRender, RenderFunction, RenderOptions} from '../render';
import {requestLogger} from '../logger';
import {metricsMiddleware as metrics} from '../metrics';
import {ping} from '../ping';
import {ValueFromContext} from '../types';

const logger = console;

Expand All @@ -18,10 +17,11 @@ interface Options {
port?: number;
assetPrefix?: string;
proxy?: boolean;
assetName?: string | ValueFromContext<string>;
Copy link
Member Author

Choose a reason for hiding this comment

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

Strictly speaking the change to assetName and renderError isn't needed as this is functionally identical, but I figure reusing the existing type is nicer than having to repeat the declarations.

assetName?: RenderOptions['assetName'];
htmlProps?: RenderOptions['htmlProps'];
serverMiddleware?: compose.Middleware<Context>[];
render: RenderFunction;
renderError?: RenderFunction;
renderError?: RenderOptions['renderError'];
app?: Koa;
}

Expand Down Expand Up @@ -50,6 +50,7 @@ export function createServer(options: Options): Server {
renderError,
serverMiddleware,
assetName,
htmlProps,
proxy = false,
app = new Koa(),
} = options;
Expand All @@ -65,7 +66,9 @@ export function createServer(options: Options): Server {
app.use(compose(serverMiddleware));
}

app.use(createRender(render, {assetPrefix, assetName, renderError}));
app.use(
createRender(render, {assetPrefix, assetName, renderError, htmlProps}),
);

return app.listen(port, ip, () => {
logger.log(`started react-server on ${ip}:${port}`);
Expand Down
31 changes: 31 additions & 0 deletions packages/react-server/src/server/test/server.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,37 @@ describe('createServer()', () => {
);
});

it('passes htmlProps through to the Html component', async () => {
function MockApp() {
return <div>markup</div>;
}

const wrapper = await saddle((port, host) =>
createServer({
port,
ip: host,
render: () => <MockApp />,
htmlProps: {
scripts: [{path: '/extraScript.js'}],
styles: [{path: '/extraStyle.css'}],
headMarkup: <script>let ScriptFromHeadMarkup = 1;</script>,
},
}),
);

const response = await wrapper.fetch('/');

await expect(response).toHaveBodyText(
`<script type="text/javascript" src="/extraScript.js" crossorigin="anonymous" defer="">`,
);
await expect(response).toHaveBodyText(
`<link rel="stylesheet" type="text/css" href="/extraStyle.css" crossorigin="anonymous"/>`,
);
await expect(response).toHaveBodyText(
`<script>let ScriptFromHeadMarkup = 1;</script>`,
);
});

it('supports updatable meta components', async () => {
const myTitle = 'Shopify Mock App';

Expand Down