Support configuring Html props from react-server #1661

BPScott · 2020-10-22T21:47:18Z

Description

react-html's <Html> component supports a bunch of props for configuring
content to inject into the head, however previously that was not exposed
from react-server's createRender and createServer functions.

This commit lets you pass a htmlProps option into createRender and
createServer that will then be applied when createRender calls <Html>.

This means you can use createServer, and inject arbitary content into
the head using <Html>'s headMarkup prop (and use any of <Html>'s other
props too):

createServer({
  render: () => <MockApp />,
  htmlProps: {
    scripts: [{path: '/extraScript.js'}],
    styles: [{path: '/extraStyle.css'}],
    headMarkup:  <>
      <script>let ScriptFromHeadMarkup = 1;</script>
      <script>let ScriptFromSecondHeadMarkup = 1;</script>
    </>,
  },
}),

To test

An hand-cranked end-to-end test of this might be hard as there probably aren't many demo apps lying around.

Find an application that uses createServer within it's server entrypoint (brand-registry does this but I'm otherwise unfamiliar with the app). Add 'htmlProps' to its list of options, and then note that the markup is injected into the head:

htmlProps: {
  headMarkup:  <>
    <script>let ScriptFromHeadMarkup = 1;</script>
  </>,
}

Fortunately the createServer tests act as automated end-to-end test by configuring the server and asserting what html gets returned. this new unit test demonstrates adding htmlProps to the call to createServer and asserting that the returned html includes that head markup.

So the real tophat instructions are "check tests pass, check the added unit tests showcase e2e behaviour" :D

Type of change

@shopify/react-html Patch: Bug (non-breaking change which fixes an issue)
@shopify/react-server Minor: New feature (non-breaking change which adds functionality)

Checklist

I have added a changelog entry, prefixed by the type of change noted above (Documentation fix and Test update does not need a changelog as we do not publish new version)

react-html's <Html> component supports a bunch of props for configuring content to inject into the head, however previously that was not exposed from react-server's createRender and createServer functions. This commit lets you pass a htmlProps option into createRender and createServer that will then be applied to the Html element. This means you can use createServer, and inject arbitary content into the head using <Html>'s headMarkup prop (and use any of <Html>'s other props too)

BPScott · 2020-10-22T21:59:25Z

packages/react-html/src/server/components/Html.tsx

  manager?: HtmlManager;
  hydrationManager?: HydrationManager;
-  children: React.ReactElement<any> | string;
+  children?: React.ReactElement<any> | string;


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

BPScott · 2020-10-22T22:00:26Z

packages/react-server/src/render/render.tsx

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

+      styles.push(...additionalStyles);


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.

BPScott · 2020-10-22T22:03:00Z

packages/react-server/src/server/server.ts

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


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.

packages/react-html/CHANGELOG.md

kaelig · 2020-10-22T23:04:36Z

Would it make sense to add an example in the README, showing what's now possible to do?

BPScott · 2020-10-23T16:41:38Z

@kaelig I added to the README section for what createServer accepts:

https://github.com/Shopify/quilt/pull/1661/files#diff-1557c7c2c28cba8b284d1990ba4ad33ddf799098943c401e2cc307886d5c5ff3R90

ismail-syed · 2020-10-23T20:25:56Z

packages/react-server/src/render/render.tsx

+      styles: additionalStyles = [],
+      ...additionalHtmlProps
+    } =
+      typeof htmlPropsInput === 'function'


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.

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

ismail-syed

might be hard as there probably aren't many demo apps lying around.

A majority of our createServer use cases are abtracted behind the react-server webpack plugin. I don't foresee any impact on those cases with this PR (I didn't tophat). 🎩ing this is challenging without a beta release of some sort. If you feel confident with these changes, that's 👍 with me. If you want to run beta release, I can give this a 🎩 .

packages/react-server/CHANGELOG.md

packages/react-server/README.md

kaelig

Thank you for the README update, I approve the README side of things! 👍

Co-authored-by: Kaelig Deloumeau-Prigent <[email protected]>

BPScott force-pushed the html-config-from-react-server branch from c3d21b4 to c0736df Compare October 22, 2020 21:52

BPScott requested review from ismail-syed and alexandcote October 22, 2020 21:55

BPScott commented Oct 22, 2020

View reviewed changes

kaelig reviewed Oct 22, 2020

View reviewed changes

packages/react-html/CHANGELOG.md Show resolved Hide resolved

kaelig reviewed Oct 22, 2020

View reviewed changes

packages/react-html/CHANGELOG.md Show resolved Hide resolved

ismail-syed reviewed Oct 23, 2020

View reviewed changes

ismail-syed approved these changes Oct 23, 2020

View reviewed changes

kaelig reviewed Oct 23, 2020

View reviewed changes

packages/react-server/CHANGELOG.md Outdated Show resolved Hide resolved

kaelig reviewed Oct 23, 2020

View reviewed changes

packages/react-server/README.md Outdated Show resolved Hide resolved

kaelig approved these changes Oct 23, 2020

View reviewed changes

BPScott and others added 2 commits October 23, 2020 15:44

Apply suggestions from code review

d07550f

Co-authored-by: Kaelig Deloumeau-Prigent <[email protected]>

Merge branch 'master' into html-config-from-react-server

154075a

BPScott merged commit a3265bf into master Oct 24, 2020

BPScott deleted the html-config-from-react-server branch October 24, 2020 01:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support configuring Html props from react-server #1661

Support configuring Html props from react-server #1661

BPScott commented Oct 22, 2020 •

edited

Loading

BPScott Oct 22, 2020

BPScott Oct 22, 2020

BPScott Oct 22, 2020

kaelig commented Oct 22, 2020

BPScott commented Oct 23, 2020

ismail-syed Oct 23, 2020

kaelig Oct 23, 2020

ismail-syed left a comment

kaelig left a comment

Support configuring Html props from react-server #1661

Support configuring Html props from react-server #1661

Conversation

BPScott commented Oct 22, 2020 • edited Loading

Description

To test

Type of change

Checklist

BPScott Oct 22, 2020

Choose a reason for hiding this comment

BPScott Oct 22, 2020

Choose a reason for hiding this comment

BPScott Oct 22, 2020

Choose a reason for hiding this comment

kaelig commented Oct 22, 2020

BPScott commented Oct 23, 2020

ismail-syed Oct 23, 2020

Choose a reason for hiding this comment

kaelig Oct 23, 2020

Choose a reason for hiding this comment

ismail-syed left a comment

Choose a reason for hiding this comment

kaelig left a comment

Choose a reason for hiding this comment

BPScott commented Oct 22, 2020 •

edited

Loading