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

NickColley · 2020-05-05T11:27:16Z

Continued in this pull request: #812

NickColley · 2020-05-05T11:29:55Z

packages/dotcom-ui-flags/src/components/FlagsEmbed.ts

@@ -1,4 +1,5 @@
-import React from 'react'
+// TODO: Figure out why importing the package name does not work.
+import { ClientEmbed } from '../../../dotcom-ui-client-embed/src'


I'm going to have another go at this but I could not figure out why referencing the package name does not work.

I think this was because when I added these as a dependency I just had not ran npm install on the directory.

I will pull your branch and have a play.

Ah yes, that tripped me up too. If there are packages with dependencies on other packages then you need to install each of them (in order, I would imagine).

NickColley · 2020-05-05T11:31:37Z

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

@@ -3,14 +3,8 @@ import subject from '../../server/formatFlagsJSON'
 const fixture = Object.freeze({ foo: 1, bar: false, baz: 'qux' })

 describe('dotcom-ui-flags/src/server/formatFlagsJSON', () => {
-  it('returns a stringified object', () => {


I've removed this since it seemed unnecessary to check the type when we do a strict assertion on an object below. Let me know what you'd prefer to do.

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

@@ -11,5 +11,5 @@ export default function formatFlagsJSON(flags: TFlagsData = {}): string {
    }
  })

-  return JSON.stringify(output)
+  return output


andygout

This feels like it is on the right track.

I guess one of the obvious things to add to the example app would be some custom data that gets passed from server to client.

The use case for this requirement is next-video-page: https://www.ft.com/video. You can see that it is using an attribute that is denied by the appContext schema: permutiveContext (hence with it is on v0.6 and not v0.7, the latter of which validates the appContext schema and fails if there are any disallowed attributes present).

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

@@ -11,5 +11,5 @@ export default function formatFlagsJSON(flags: TFlagsData = {}): string {
    }
  })

-  return JSON.stringify(output)
+  return output


andygout · 2020-05-05T12:01:16Z

packages/dotcom-ui-flags/src/components/FlagsEmbed.ts

@@ -1,4 +1,5 @@
-import React from 'react'
+// TODO: Figure out why importing the package name does not work.
+import { ClientEmbed } from '../../../dotcom-ui-client-embed/src'


I will pull your branch and have a play.

NickColley · 2020-05-05T12:13:37Z

Could you explain how you're testing this with the front page?

Hmm. Both flags and appContext already seem to be in object form (rather than a stringified object), which is surprising.

npm link?

andygout · 2020-05-05T12:16:51Z

Could you explain how you're testing this with the front page?

Just checking the Dev Tools Elements tab on production ft.com, which is why it was surprising that the appContext and flags data is not stringified (as code suggests we are currently stringifying it).

NickColley · 2020-05-05T12:48:53Z

examples/kitchen-sink/client/main.js


 readyState.domready.then(() => {
  const flagsClient = flags.init()
  const appContextClient = appContext.init()

+  const customContext = loadClientEmbed('custom-context')


As you can see it's simple at the moment but does not follow the init convention elsewhere + the getAll pattern.

Should we require the user to pass their ID to init or automagically find clientEmbeds when init() is called and then allow them to use the client to specify an embed?

Would be nice if we can keep it matching the pattern.

I think including the ID as an argument for init will make it clear which embed is being retrieved, but means it will require an individual DOM query for each embed being retrieved (although that is what the code is currently doing, and there is only one extra embed being added to one app at the moment, so this is minimal).

One other alternative would be this sort of pattern:

const clientEmbed = new clientEmbed('IDHERE') clientEmbed.init()

@pixelandpage would appreciate your thoughts on what the interface could look like here as well if you have time :)

NickColley · 2020-05-05T12:49:37Z

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

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


In documentation should make it clear to put the embed at the bottom of the page to remove risk of it blocking page render.

NickColley · 2020-05-06T10:47:09Z

examples/kitchen-sink/client/main.js


 readyState.domready.then(() => {
  const flagsClient = flags.init()
  const appContextClient = appContext.init()

+  const clientEmbedClient = clientEmbed.init('custom-context')


Probably should move this 'custom-context' constant into a 'constants.js' file as per convention in this repo...

NickColley · 2020-05-06T10:47:55Z

packages/dotcom-ui-app-context/src/client/AppContext.ts

-
-  getAll(): TAppContext {
-    return this.appContext
+    super(options.appContext)


Need to figure out how to extend a class and apply the types, as this currently has removed the type information.

Looks like what I wanted to do is not possible(?) so will need to figure out another way to achieve this: microsoft/TypeScript#3402

NickColley · 2020-05-06T10:48:58Z

packages/dotcom-ui-client-embed/src/client/ClientEmbedData.ts

@@ -0,0 +1,15 @@
+export default class ClientEmbedData {


Any way to add types here and then make them more specific when the class gets extended?

NickColley · 2020-05-06T10:54:40Z

packages/dotcom-ui-client-embed/src/client/ClientEmbedData.ts

+  private data
+
+  constructor(data) {
+    this.data = Object.freeze(data)


Since the data in this is likely to be nested, should we consider doing a deeper freeze? may not be worth the extra code required in the bundle...

NickColley · 2020-05-07T09:09:18Z

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

@@ -0,0 +1,2 @@
+export * from './components/ClientEmbed'
+export * from './client'


I've had to add the client entrypoints to the main entrypoints which I assume is not ideal since we have a separate browser entrypoint for a reason. This was necessary to allow the client code to be shared in other packages with the TypeScript types working...

NickColley · 2020-05-07T10:02:08Z

Something I noticed:

#809

NickColley · 2020-05-11T11:33:13Z

I have ruled out having a default 'id' prop as I'm not sure there's an easy way to communicate you need unique IDs if you use more than one embed.

NickColley · 2020-05-11T14:58:55Z

Spoke to Maggie, we're gonna go with data-embed to clear up the weirdness with ClientEmbedClient class.

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.

Allow users to create an instance of the client and initialise it later. Don't expose internal methods and have a consistent 'init' method to use.

NickColley · 2020-05-11T16:10:44Z

I suggested refactoring away from .init() with arguments since it felt weird but then since found that nTracking and nTracking do it. So perhaps we should go back to it...!

NickColley · 2020-05-11T16:39:01Z

I've spiked putting this into next-video-page: https://github.com/Financial-Times/next-video-page/compare/spike-data-embed-2?expand=1

I'm curious if we're really solving the underlying problem. If our main worries were that app context was being passed into nTracking etc should we not have validation or an allowlist there? And allow for app context to be used for all data, which is as we've seen, what people tend to do?

If when we passed the app context into things like nTracking, nTracking allowlisting only the properties it expects, would this allow us to use AppContext in a broader way?

andygout · 2020-05-12T07:01:56Z

This PR description goes some way to answering that question: #782.

I think it's preferable to enforce the schema as far upstream as possible so that Page Kit - being the originator of the appContext object - is able to enforce the properties it contains, ensuring that they are not outside the scope of its definition.

It's fair to say that (in the case of next-video-page) developers had used the appContext object for other data simply because it was a pre-existing way of doing what they required and therefore the most convenient solution. Even though the attribute added in next-video-page is called permutiveContext, it is just an ID and API key, which are there to allow the app run rather than serving the purpose of providing context about the app.

The flags object should only contain data about flags and by the same virtue the appContext object should only contain data about the app's context.

The concern would be that appContext would become a dumping ground for all sorts of properties and have an inconsistent series of properties across apps.

If it transpires that a property should be added to appContext, then this is something on which Page Kit can make the decision by making an allowance in its JSON schema validation.

NickColley · 2020-05-12T09:42:05Z

@andygout thanks for the clarity, let's continue. I'm still getting my head around this space. :)

NickColley · 2020-05-12T11:19:13Z

Worked on a different branch to clean up renaming files: #812

NickColley requested a review from a team May 5, 2020 11:29

NickColley commented May 5, 2020

View reviewed changes

NickColley linked an issue May 5, 2020 that may be closed by this pull request

Create a dotcom-ui-client-embed package for sharing data between the server and the client #799

Closed

NickColley commented May 5, 2020

View reviewed changes

andygout reviewed May 5, 2020

View reviewed changes

magsallen added Client-side integration Refactor Strategy labels May 5, 2020

NickColley commented May 5, 2020

View reviewed changes

NickColley commented May 6, 2020

View reviewed changes

NickColley commented May 7, 2020

View reviewed changes

NickColley force-pushed the dotcom-ui-client-embed branch from a764a46 to e891ef7 Compare May 7, 2020 13:19

NickColley force-pushed the dotcom-ui-client-embed branch 2 times, most recently from 76dc970 to 1a5275c Compare May 11, 2020 15:13

Nick Colley and others added 6 commits May 11, 2020 16:15

Add boilerplate for dotcom-ui-client-embed package

8b39a0b

Add reusable ClientEmbed component

5c4a1d0

Refactor AppContextEmbed to use shared ClientEmbed

b010952

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

Refactor FlagsEmbed to use shared ClientEmbed

a0b4240

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

Add loadClientEmbed

834f67f

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

NickColley force-pushed the dotcom-ui-client-embed branch from 1a5275c to 8d904d8 Compare May 11, 2020 15:16

Nick Colley added 11 commits May 11, 2020 16:31

Refactor loadAppContext to use loadClientEmbed

cc6192d

Refactor loadFlags to use loadClientEmbed

2f26db7

Add client embed as a dependency for app context

fa4738e

Add client embed as a dependency for flags

8958580

Add custom context example to kitchen sink

289081e

Add ClientEmbedData class for safer data retrieval

18989fe

Mirrors the classes found in flags and app-context.

Add fleshed out init methods and expose methods

b6ee9c0

Update kitchen sink example to demonstrate new exposed methods

5243d11

Refactor client embed id into a shared constant

3b9a944

Add ClientEmbedClient class

d4f740b

Allow users to create an instance of the client and initialise it later. Don't expose internal methods and have a consistent 'init' method to use.

Move ClientEmbed tests into components/ folder

6f9fe16

NickColley force-pushed the dotcom-ui-client-embed branch from 8d904d8 to 6f9fe16 Compare May 11, 2020 15:31

NickColley changed the title ~~Add dotcom-ui-client-embed~~ Add dotcom-ui-data-embed May 11, 2020

Rename dotcom-ui-client-embed to dotcom-ui-data-embed

aa8683c

NickColley force-pushed the dotcom-ui-client-embed branch from 8c51043 to aa8683c Compare May 11, 2020 16:00

Refactor away from class to mirror nTracking

7907e42

NickColley closed this May 12, 2020

NickColley deleted the dotcom-ui-client-embed branch May 12, 2020 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

NickColley commented May 5, 2020 •

edited

Loading

NickColley May 5, 2020

NickColley May 5, 2020

andygout May 5, 2020

andygout May 5, 2020

NickColley May 5, 2020

This comment was marked as resolved.

This comment was marked as resolved.

andygout left a comment

This comment was marked as resolved.

andygout May 5, 2020

NickColley commented May 5, 2020 •

edited

Loading

andygout commented May 5, 2020

NickColley May 5, 2020

andygout May 5, 2020

NickColley May 5, 2020 •

edited

Loading

NickColley May 5, 2020

NickColley May 6, 2020

NickColley May 6, 2020

NickColley May 7, 2020

NickColley May 6, 2020

NickColley May 6, 2020

NickColley May 7, 2020

NickColley commented May 7, 2020

NickColley commented May 11, 2020

NickColley commented May 11, 2020

NickColley commented May 11, 2020

NickColley commented May 11, 2020 •

edited

Loading

andygout commented May 12, 2020 •

edited

Loading

NickColley commented May 12, 2020

NickColley commented May 12, 2020

		@@ -0,0 +1,2 @@
		export * from './components/ClientEmbed'
		export * from './client'

Add dotcom-ui-data-embed #808

Add dotcom-ui-data-embed #808

Conversation

NickColley commented May 5, 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

This comment was marked as resolved.

This comment was marked as resolved.

andygout left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

NickColley commented May 5, 2020 • edited Loading

andygout commented May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickColley May 5, 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 commented May 7, 2020

NickColley commented May 11, 2020

NickColley commented May 11, 2020

NickColley commented May 11, 2020

NickColley commented May 11, 2020 • edited Loading

andygout commented May 12, 2020 • edited Loading

NickColley commented May 12, 2020

NickColley commented May 12, 2020

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

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

NickColley commented May 5, 2020 •

edited

Loading

NickColley commented May 5, 2020 •

edited

Loading

NickColley May 5, 2020 •

edited

Loading

NickColley commented May 11, 2020 •

edited

Loading

andygout commented May 12, 2020 •

edited

Loading