Add `dotcom-ui-data-embed` #812

NickColley · 2020-05-12T11:17:49Z

In order to allow users to share data between the server and client introduce a new dotcom-ui-data-embed package.

Some parts of this package can be reused and consumed by the dotcom-ui-flags and dotcom-ui-app-context packages.

I have spiked this feature in the next-video-page application that was blocked without this feature.

Screenshot of kitchen sink example output

Closes #799
Closes #761

andygout

A nice, clean implementation. 👍

And I think the naming decisions ended well as this (to me at least) all reads really clearly.

I have added a few comments.

Is it possible to add a screengrab from Dev Tools of some dummy data being embedded in the DOM?

I am also going to have a go of running this branch locally.

andygout · 2020-05-12T12:47:32Z

examples/kitchen-sink/server/controllers/home.js

@@ -64,6 +67,7 @@ module.exports = (_, response, next) => {
            </section>
          </div>
        </Layout>
+        <DataEmbed id={DATA_EMBED_ID} data={{ foo: true, bar: 'qux' }} />


Is there a reason this is not passed as an argument to Shell like flags and appContext? Would be good to keep the same pattern if possible.

In the Shell component, flags and appContext end up at the bottom of the body tags so as to be non-blocking: https://github.com/Financial-Times/dotcom-page-kit/blob/master/packages/dotcom-ui-shell/src/components/Shell.tsx#L76-L77. Is there a difference between the DataEmbed being here rather than there?

There can be multiple instances of DataEmbed if a user wants, so right now it's up to them how to get data to DataEmbed.

In theory we could replicate the middleware and server packages of app-context but that felt more than what was initially agreed in the original issue.

Then DataEmbed would just be a generic AppContext.

In theory we could replicate the middleware and server packages

Feels like over-engineering? 😬 We don't expect many apps to interact with this tool directly

There can be multiple instances of DataEmbed

So using this usage of Page Kit as an example, it would look something like:

const document = React.createElement( Shell, { ...shellProps }, [ React.createElement(Layout, { ...layoutProps, contents: html }), React.createElement(ClientEmbed, id='custom-context-foobar' data={{ foo: true, bar: 'qux' }}), React.createElement(ClientEmbed, id='custom-context-barfoo' data={{ foo: false, bar: 'quux' }}) ] );

This will require the document element created by the consuming app to have that many more arguments (at the moment it is quite clean with just Shell and Layout), but admittedly it is currently only one app that will be using ClientEmbed so is not a huge consideration.

In keeping to the principle of least surprise, users may well expect for all the client-embedded data (flags, appContext, custom) to sit alongside one another, and that this could be achieved by passing the custom data as part of the props given to the Shell component - is this easily possible?

is this easily possible?

I think it is, but we'd want to mirror the dotcom-server-app-context and dotcom-middleware-app-context package functionality so that it is integrated in a similar way to app context which we discussed above.

So we could go ahead and flesh that functionality out, or intentionally keep this simpler and observe how people use the package before committing to something in page kit.

👍 Okay - let's keep it as is and monitor.

andygout · 2020-05-12T12:49:59Z

packages/dotcom-ui-data-embed/README.md

@@ -0,0 +1,88 @@
+# @financial-times/dotcom-ui-data-embed
+
+This package provides methods for putting data into your server-side rendered pages and safely retrieving it again in the browser.


Suggested change

This package provides methods for putting data into your server-side rendered pages and safely retrieving it again in the browser.

This package provides methods for putting data into your server-side rendered pages and retrieving it again in the browser.

Does the issue of safety factor here? (Honest question - I could have easily missed something.)

This is me mirroring language in app-context and flags, since the extra safety comes from the DataEmbedStore class rather than returning a regular object.

andygout · 2020-05-12T12:55:51Z

packages/dotcom-ui-data-embed/README.md

+
+const dataEmbedClient = dataEmbed.init({ id: 'data-embed' })
+
+if (dataEmbedClient.get('property')) {


Are flags and appContext also frozen, i.e. would this be a breaking change?

Would have deep-freezing been an option (as opposed to just freezing the top-level properties) and offered any benefits for nested objects that may be used in the future?

I think I'm mostly worried about the performance cost of shipping a deep-freeze library and I'm not too sure of the consequences of us not doing this. I suppose we want to encourage people to make copies of the data rather than relying on the ability to mutate it deeply...

Should we investigate the bundle cost of doing a deep-freeze?

In terms of freezing flags and appContext, that should be unchanged as they don't reuse the DataEmbedStore.

Let's keep it just as a .freeze as flags and appContext currently are and worry about it if the nested properties being unfrozen ever becomes an issue.

I asked more for my curiosity.

andygout · 2020-05-12T13:33:11Z

packages/dotcom-ui-data-embed/src/client/DataEmbedStore.ts

+  }
+
+  get(key: string) {
+    return this.data.hasOwnProperty(key) ? this.data[key] : undefined


Wouldn't this.data[key] return undefined if the key does not exist as a property on this.data? I.e. is the conditional check necessary here?

I'll admit a lot of this code is me looking for patterns in flags and app-context and consolidating the logic.

So while I didn't make the decision, I can only assume it's to avoid you being able to try to 'get' properties built into the Object.prototype e.g. constructor or __proto__.

So I'm not too sure, but perhaps could be simplified alongside flags and app-context later on?

https://masteringjs.io/tutorials/fundamentals/hasownproperty

There is always the assumption of good faith that there is a good reason behind any pre-existing code. Also happy to leave it as is for now.

You're right, definitely should have checked this, thanks for the prompt. :)

andygout · 2020-05-12T13:37:01Z

packages/dotcom-ui-data-embed/src/client/index.ts

+import loadDataEmbed from './loadDataEmbed'
+import DataEmbedStore from './DataEmbedStore'
+
+const init = ({ id }: { id: string }) => {


Not sure if it's convention, but are index files usually for just listing the modules by importing and exporting them? I.e. should this function live in its own file and be imported?

I say this when it could easily be possible that we have functions in index files elsewhere in this repo. Am just thinking out loud.

This follows what app-context and flags are doing. I agree it's a bit odd though.

Fair enough - happy to leave as is.

andygout · 2020-05-12T13:46:16Z

packages/dotcom-server-app-context/src/__test__/validate.spec.ts

+Validation error: data should NOT have additional properties, received "isNotInTheSchema"
+
+If you want to share application specific data with the client, consider using @financial-times/dotcom-ui-data-embed.
+`.trim()


Backticks can be frustrating. I wonder if there's a way to do multiline without inconsistent formatting and trim()… 🤔

We could consider adding a dependency like https://www.npmjs.com/package/outdent but I felt like that was a bit much for this small usecase. What do you think?

We could always just do something like

'string\n' + 'second-string'

or

[ 'string', 'second-string' ].join('\n')

Yeah, I agree with your feeling about importing a package just for this.

You could even have one really long line…

None is particularly nice so I will leave it to you to decide (which may as well be how it is currently).

andygout · 2020-05-12T13:46:35Z

packages/dotcom-server-app-context/src/validate.ts

+
+If you want to share application specific data with the client, consider using @financial-times/dotcom-ui-data-embed.
+`.trim()
+    )


Same backticks issue as above.

When I looked at this again I noticed there was an opportunity to improve the error message so we only output the additional information when they specifically encounter the error for adding additional properties. So I've managed to address the odd formatting as well at the same time...

magsallen

Excellent work! I've added a few small comments but it's looking really good

packages/dotcom-ui-data-embed/README.md

magsallen · 2020-05-12T15:20:25Z

packages/dotcom-ui-data-embed/README.md

+
+### Client-side integration
+
+Once you have you data embedded in your page you can use the [data embed client](#client-side-api) in your client-side code. The client provides methods for safely retrieving the status of individual context properties.


Should individual context properties be individual data properties? Just wondering if the terminology has shifted since this was written

packages/dotcom-ui-data-embed/README.md

magsallen · 2020-05-12T15:35:42Z

packages/dotcom-ui-data-embed/README.md

+
+Requires an `id` parameter within an options object, this `id` should match your embed `id` within the page.
+
+## data embed client API


I'm not sure it's super obvious what data embed client API means here. Would something like Component API work or have I misunderstood the structure?

packages/dotcom-ui-flags/src/server/formatFlagsJSON.ts

packages/dotcom-server-app-context/README.md

magsallen · 2020-05-12T16:21:48Z

examples/kitchen-sink/server/controllers/home.js

@@ -64,6 +67,7 @@ module.exports = (_, response, next) => {
            </section>
          </div>
        </Layout>
+        <DataEmbed id={DATA_EMBED_ID} data={{ foo: true, bar: 'qux' }} />


In theory we could replicate the middleware and server packages

Feels like over-engineering? 😬 We don't expect many apps to interact with this tool directly

magsallen · 2020-05-12T16:47:42Z

The spike in next-video-page looks good 👌 Once these changes are released we should ask the Content Innovation team to test/review the new approach in that app, then we can refine this package if needed before rolling v0.8.x out more widely.

Since `dotcom-ui-app-context` and `dotcom-ui-flags` share functionality this commit moves JSON output into the embed component which is the same as how it works in app-context. This'll hopefully make it easier to use the new package in the future. May be a breaking change if formatFlagsJSON is part of the public API.

Also rename the file since there's no JSX syntax used in this file anymore.

This allows users to parse the data embedded in the page.

Mirrors the classes found in flags and app-context. Expose through fleshed out init method.

magsallen · 2020-05-14T12:58:23Z

packages/dotcom-ui-data-embed/README.md

+
+### `init({ id }:{ id: string })`
+
+Initialises and returns a new [data embed client](#data-embed-client-api) which can be used to safely access the status of individual contexts.


Would 'properties' or 'values' work in place of 'contexts' here? I'd rather not introduce any confusion with appContext, and calling things 'context' in the past has called all manner of confusion 🙈

magsallen · 2020-05-14T12:58:42Z

packages/dotcom-ui-data-embed/src/__test__/client/DataEmbedStore.spec.ts

+      expect(instance.get('baz')).toBe('qux')
+    })
+
+    it('returns undefined for contexts which do not exist', () => {


See pedantic 'contexts' comment above

Add boilerplate for dotcom-ui-data-embed package

0052901

NickColley mentioned this pull request May 12, 2020

Add dotcom-ui-data-embed #808

Closed

NickColley marked this pull request as ready for review May 12, 2020 12:38

NickColley requested a review from a team as a code owner May 12, 2020 12:38

andygout reviewed May 12, 2020

View reviewed changes

magsallen added Refactor Strategy labels May 12, 2020

NickColley force-pushed the dotcom-ui-data-embed branch from 52394dd to ed8d922 Compare May 12, 2020 16:17

magsallen reviewed May 12, 2020

View reviewed changes

NickColley force-pushed the dotcom-ui-data-embed branch from 3204cf6 to f34248d Compare May 13, 2020 09:09

Nick Colley added 12 commits May 13, 2020 10:20

Add reusable DataEmbed component

88c9277

Refactor AppContextEmbed to use shared DataEmbed

8d6bc5a

Also rename the file since there's no JSX syntax used in this file anymore.

Refactor FlagsEmbed to use shared DataEmbed

288c7c3

Also rename the file since there's no JSX syntax used in this file anymore.

Add loadDataEmbed

4eb4e39

This allows users to parse the data embedded in the page.

Refactor loadAppContext to use loadDataEmbed

bf1b5f7

Refactor loadFlags to use loadDataEmbed

20e69a3

Add DataEmbedStore class for safer data retrieval

60f4343

Mirrors the classes found in flags and app-context. Expose through fleshed out init method.

Add kitchen sink example for dotcom-ui-data-embed package

833a000

Update dotcom-server-app-context validate to reference data-embed

85fd3df

Add documentation for dotcom-ui-data-embed

0c9c451

Reference data-embed package in app-context documentation.

ff280ae

NickColley force-pushed the dotcom-ui-data-embed branch from f34248d to ff280ae Compare May 13, 2020 09:21

andygout approved these changes May 14, 2020

View reviewed changes

magsallen reviewed May 14, 2020

View reviewed changes

Nick Colley added 2 commits May 14, 2020 14:03

Add integration test for data embed in kitchen sink

0315037

Remove references to context in data embed package.

9bc6437

NickColley merged commit 4be5e22 into master May 14, 2020

NickColley deleted the dotcom-ui-data-embed branch May 14, 2020 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `dotcom-ui-data-embed` #812

Add `dotcom-ui-data-embed` #812

NickColley commented May 12, 2020 •

edited

Loading

andygout left a comment

andygout May 12, 2020

NickColley May 12, 2020

magsallen May 12, 2020

andygout May 13, 2020

NickColley May 13, 2020

andygout May 13, 2020

andygout May 12, 2020

andygout May 12, 2020

NickColley May 12, 2020

andygout May 12, 2020

NickColley May 12, 2020

NickColley May 12, 2020

andygout May 12, 2020

andygout May 12, 2020

NickColley May 12, 2020 •

edited

Loading

andygout May 12, 2020

NickColley May 12, 2020

andygout May 12, 2020

NickColley May 12, 2020

andygout May 12, 2020

andygout May 12, 2020

NickColley May 12, 2020

NickColley May 12, 2020 •

edited

Loading

andygout May 12, 2020

andygout May 12, 2020

NickColley May 12, 2020

magsallen left a comment

magsallen May 12, 2020

magsallen May 12, 2020

magsallen May 12, 2020

magsallen commented May 12, 2020

magsallen May 14, 2020

magsallen May 14, 2020

		@@ -0,0 +1,88 @@
		# @financial-times/dotcom-ui-data-embed

		This package provides methods for putting data into your server-side rendered pages and safely retrieving it again in the browser.


		const dataEmbedClient = dataEmbed.init({ id: 'data-embed' })

		if (dataEmbedClient.get('property')) {


		### Client-side integration

		Once you have you data embedded in your page you can use the [data embed client](#client-side-api) in your client-side code. The client provides methods for safely retrieving the status of individual context properties.


		Requires an `id` parameter within an options object, this `id` should match your embed `id` within the page.

		## data embed client API


		### `init({ id }:{ id: string })`

		Initialises and returns a new [data embed client](#data-embed-client-api) which can be used to safely access the status of individual contexts.

Add dotcom-ui-data-embed #812

Add dotcom-ui-data-embed #812

Conversation

NickColley commented May 12, 2020 • edited Loading

Screenshot of kitchen sink example output

andygout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickColley May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickColley May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magsallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magsallen commented May 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `dotcom-ui-data-embed` #812

Add `dotcom-ui-data-embed` #812

NickColley commented May 12, 2020 •

edited

Loading

NickColley May 12, 2020 •

edited

Loading

NickColley May 12, 2020 •

edited

Loading