feat(common): new subscription model #260

johannes-lindgren · 2023-08-23T11:08:02Z

What?

This PR introduces callback queue

Field Plugin attaches callbackId to messages to Container
Container responds with a message including callbackId
Field Plugin executes the callback only if the callbackId matches.

This enables us to

have multiple instances of createFieldPlugin()
remove state management out of the library and let Storyfront keep merging states and deliver the full state back to Field Plugin

Why?

JIRA: EXT-1957

How to test? (optional)

vercel · 2023-08-23T11:08:05Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
plugin-sandbox	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 6, 2023 2:28pm

johannes-lindgren · 2023-08-23T14:48:13Z

packages/field-plugin/src/createFieldPlugin/createPluginActions/createPluginActions.ts

-  let assetSelectedCallbackRef: undefined | ((filename: Asset) => void) =
-    undefined
-  let assetSelectedCallbackId: undefined | string = undefined
+  const { pushCallback, popCallback } = callbackQueue()


This function returns two functions that lets you manage callback functions.

When pushing, you specify the type of the callback function, and the callback function itself. It returns an id that you can include in the message to the visual editor.

When popping, you specify the callbackId and the type of the callback function, this way, typescript is able to guarantee that the callback function is of the expected type.

johannes-lindgren · 2023-08-23T14:49:02Z

packages/field-plugin/src/createFieldPlugin/createPluginActions/createPluginActions.ts

  const onStateChange: OnStateChangeMessage = (data) => {
-    state = {
-      ...state,
-      ...partialPluginStateFromStateChangeMessage(data),
-    }
-    onUpdateState(state)
+    popCallback('state', data.callbackId)?.(data)
+    onUpdateState(pluginStateFromStateChangeMessage(data))
+  }
+  const onLoaded: OnLoadedMessage = (data) => {
+    onUpdateState(pluginStateFromStateChangeMessage(data))
  }


This is a new message. Whenever the state in the field plugin editor changes, it will send an event with the new state to the field plugin. The state is identical to the state in the loaded message.

Two things happen:

onUpdateState is called, as before, except we no longer need to store any state, since isModalOpen is included.

The callback function that is associated with the callbackId in the message is invoked (if found)

johannes-lindgren · 2023-08-23T14:54:32Z

packages/field-plugin/src/createFieldPlugin/createPluginActions/createPluginActions.ts

@@ -114,45 +96,42 @@ export const createPluginActions: CreatePluginActions = (

  return {
    actions: {


As seen here, all actions return a promise that resolves when a confirmation of the message has been received.

I love how these actions are uniform now.

johannes-lindgren · 2023-08-23T14:55:58Z

packages/field-plugin/src/messaging/pluginMessage/containerToPluginMessage/LoadedMessage.ts

I renamed this from StateChangedMessage to LoadedMessage, because the StateChangedMessage is now referring to a different message (the one that is sent during the plugin's lifecycle)

johannes-lindgren · 2023-08-23T14:57:20Z

packages/field-plugin/src/messaging/pluginMessage/containerToPluginMessage/MessageToPlugin.ts

@@ -3,6 +3,7 @@ import { hasKey } from '../../../utils'
 export type MessageToPlugin<Action extends string> = {
  action: Action
  uid: string
+  callbackId?: string


Every message has potentially a callbackId. It is optional because sometimes, the event was not triggered by the plugin, so there would be no callback to refer to

Can you give an example?

The following MessageToContainer messages resolve a promise: loaded, asset-selected, update, toggle-modal, request-context. Therefore, all these messages can contain a callbackId.

The heightChange message does not resolve a promise. Right now, I don't see a reason why it would. However, it is conceivable. Not sure if we should omit callbackId from MessageToContainer because of this.

For MessageToPlugin, all messages could resolve a promise: loaded, state-changed, asset-selected, context, so all can contain a callbackId.

Actually, right now, there is no real example of a MessageToPlugin message that does not contain a callbackId... but, I can invent one!

Let's say the user clicks outside the modal window: ideally, this would close the modal window. Upon such click event, the Visual Editor sends a state-updated message to the field plugin. The event originated from the Visual Editor, so there is no promise to resolve, therefore the callbackId is omitted.

Ah, thanks for the explanation. Yeah it makes sense to have it optional. We cannot revert it if we being with it being required :)

~~@johannes-lindgren @eunjae-lee Can you explain what how we will use the callbackId, or why does it need to be present?~~

clarified

johannes-lindgren · 2023-08-23T14:57:39Z

...ges/field-plugin/src/messaging/pluginMessage/containerToPluginMessage/StateChangedMessage.ts

@@ -18,17 +18,18 @@ export type StateChangedMessage = MessageToPlugin<'loaded'> & {
  // Related to the field type itself
  schema: FieldPluginSchema
  model: unknown
+  isModalOpen: boolean


The state changed message includes the state of the modal window

johannes-lindgren · 2023-08-23T14:58:16Z

...ages/field-plugin/src/messaging/pluginMessage/containerToPluginMessage/LoadedMessage.test.ts

Need to add tests for the new StateChangedMessage

johannes-lindgren · 2023-08-23T15:02:09Z

As you can see in the pull request, I intend to make each action return a promise, but I am not sure how to implement this in Storydfront yet; the code there is a bit messy.

eunjae-lee

I absolutely love this. Let's keep going! 🚀

eunjae-lee · 2023-08-23T15:11:59Z

packages/field-plugin/src/createFieldPlugin/createPluginActions/callbackQueue.ts

+type CallbackMap = {
+  asset: CallbackRef<AssetSelectedMessage>[]
+  context: CallbackRef<ContextRequestMessage>[]
+  state: CallbackRef<LoadedMessage>[]


Let's say

const plugin1 = createFieldPlugin(); const plugin2 = createFieldPlugin(); const plugin3 = createFieldPlugin(); ... plugin1.actions.setContent({...});

After that, the container will send the new state. In this case, I think all of plugin 1 ~ 3 should receive the state update regardless of callbackId. What do you think?

Yes, all of them would. And the promise that was created in one of them will be resolved. These lines are for handling the callback function in the promise.

~~Oh, I see. All the plugins get updated by onLoaded event, but only the caller's promise gets resolved by the callback.~~

I thought valueChangeMessage and modalChangeMessage contained the states, but they're just return value from the Container, and I assume the Container send a separate loaded event, so that all the plugins' onLoaded event gets triggered?

onLoaded is called only once after the library is loaded.

And there's onStateChange, which can be returned after setContent or setModalOpen.

https://github.com/storyblok/field-plugin/pull/260/files#diff-e62bdc8d2bfaac8dcd9c0ec5f420b868131bc32f10c4be5743ba3e5e28ebe938R49-R54

If plugin1 calls setContent, and Storyfront will stateChanged event back to all of plugin1 ~ 3. Only plugin1 will resolve the promise. However, all of plugin1 ~ 3 will still call the updater function.

popCallback('stateChanged', data.callbackId)?.(data) onUpdateState(pluginStateFromStateChangeMessage(data))

I was writing down a question, and in the middle of it, I understood. But still left it as a record for others :)

Yes, precisely 🙂

eunjae-lee · 2023-08-23T15:16:57Z

packages/field-plugin/src/createFieldPlugin/createPluginActions/callbackQueue.ts

+    callbackId: CallbackId | undefined,
+  ): CallbackMap[T][number]['callback'] | undefined => {
+    // TODO remove callback when popping
+    return callbackMap[callbackType].findLast(


I'm not sure but don't we need to find the first one?

It depends on whether we push items to the front or to the back of the queue. Since each callbackId is unique, it will find the element regardless of how it traverses the list, but it's slightly better to check from the end of the array, since the array is sorted by order of insertion.

eunjae-lee · 2023-08-23T15:46:52Z

packages/field-plugin/src/createFieldPlugin/createPluginActions/createPluginActions.ts

+            resolve(pluginStateFromStateChangeMessage(message)),
+          )
+          postToContainer(
+            valueChangeMessage({ uid, callbackId, model: content }),


it'd be nice to rename this to contentChangeMessage in the future (not in this PR). It's a note for myself :)

Yes, that is true. (@demetriusfeijoo We started with the word "value" and switched to "content")

eunjae-lee · 2023-08-23T15:48:05Z

packages/field-plugin/src/createFieldPlugin/createPluginActions/createPluginActions.ts

@@ -114,45 +96,42 @@ export const createPluginActions: CreatePluginActions = (

  return {
    actions: {


I love how these actions are uniform now.

eunjae-lee · 2023-08-25T13:57:52Z

...ges/field-plugin/src/messaging/pluginMessage/pluginToContainerMessage/PluginLoadedMessage.ts

@@ -4,6 +4,7 @@ import { hasKey } from '../../../utils'
 export type PluginLoadedMessage = MessageToContainer<'loaded'> & {
  // signals that the field plugin is responsible for its own scrolling in modal mode
  fullHeight?: boolean
+  subscribeState?: boolean


~~is it to inform something to the container?~~

Okay, got it. https://github.com/storyblok/storyfront/pull/4375/files#diff-88cca765f1678c3aabf60de51a4fbc075e55911f9934afc336c80d9b150c7d0aR242-R248

packages/field-plugin/src/createFieldPlugin/createPluginActions/createPluginActions.ts

eunjae-lee · 2023-09-04T15:03:34Z

I'd love to get your reviews @demetriusfeijoo @BibiSebi on this :)

eunjae-lee · 2023-09-05T14:28:40Z

Current dependencies on/for this PR:

main
- PR feat(common): new subscription model #260 👈
  - PR feat(lib): add parseContent #266
    - PR fix(helper): update react helper #267
      - PR fix(helper): update vue3 helper #269
        
        PR fix(helper): update vue 2 template and helper #270
        
        PR fix(demo): provide parseContent #271

This comment was auto-generated by Graphite.

BibiSebi · 2023-09-05T14:59:26Z

packages/field-plugin/src/messaging/pluginMessage/pluginToContainerMessage/GetContextMessage.ts

+  // Previously, debounced message was the default behavior.
+  // That debouncing implementation can be problematic, for example,
+  // when multiple field plugin instances request for context.
+  debounce: false


Never saw this before, is the debouncing implemented inside the storyfront?

@BibiSebi yes, but only for requestContext. The debouncing is enabled by default for requestContext, but neither Johannes nor I know why it exists. And it seems troublesome especially with multiple field plugin instances. So I've added this new property to bypass debouncing, which is used only for the Field Plugin SDK (not the legacy ones).

BibiSebi · 2023-09-05T15:01:01Z

This is huge, thank you for your efforts @johannes-lindgren and @eunjae-lee 👏

demetriusfeijoo

Hey @eunjae-lee, I've left some notes.

Let me know if they make sense.

Related to the new approach, everything look nice to me.
I didn't find any particular or possible issue.

demetriusfeijoo · 2023-09-06T11:20:55Z

packages/container/src/components/FieldPluginContainer.tsx

-          setHeight,
-          setModalOpen: setModalOpen,
+          setHeight: onHeightChange,
+          setModalOpen: onModalChange,


The same as the previous comment:

demetriusfeijoo · 2023-09-06T11:22:56Z

packages/container/src/components/FieldPluginContainer.tsx

+    }
+
+    dispatchStateChanged(stateChangedData)
+  }, [dispatchStateChanged, stateChangedData])


The same as the previous comment:

demetriusfeijoo · 2023-09-06T11:24:19Z

packages/container/src/components/FieldPluginContainer.tsx

    }),
-    [uid, content, language, schema, story],
+    [uid, content, language, schema, story, isModalOpen],


Hey @eunjae-lee, here eslint is warning for missing dependencies:

demetriusfeijoo · 2023-09-06T11:29:58Z

packages/container/src/dom/createContainerMessageListener.ts

@@ -1,24 +1,27 @@
 import {
+  AssetModalChangeMessage,
+  ContextRequestMessage,


This was imported but it's not being used @eunjae-lee.

demetriusfeijoo · 2023-09-06T11:36:33Z

packages/field-plugin/src/createFieldPlugin/createPluginActions/createPluginActions.ts

Is this note still necessary?

eunjae-lee · 2023-09-06T14:27:24Z

Thanks @demetriusfeijoo ! My vim didn't show me this lint errors 😅 And we also need to fix the CI to check these.

demetriusfeijoo

thanks for all the effort on this one @johannes-lindgren and @eunjae-lee.

Aewsome!

eunjae-lee · 2023-09-14T14:27:03Z

Merge Activity

Sep 14, 10:27 AM: @eunjae-lee started a stack merge that includes this pull request via Graphite.
Sep 14, 10:27 AM: @eunjae-lee merged this pull request with Graphite.

@johannes-lindgren

## What? This PR adds `parseContent` as the option to `createFieldPlugin()` function. This makes sure that we always pass around correctly formed payload. This is almost the exact porting of #241. @johannes-lindgren did the job there, but since #260, it was too hard to resolve conflicts. So I've created a new PR here. ## Why? JIRA: EXT-1918 more discussion on #241 ## Notice It's expected to see tests broken in this pull request, as the demo and the helpers are not taking this change into account. It will be fixed in the separate pull request.

feat(common): work-in-progress callbacks for all messages

b4df31f

johannes-lindgren self-assigned this Aug 23, 2023

vercel bot had a problem deploying to Preview August 23, 2023 11:08 Failure

johannes-lindgren added 3 commits August 23, 2023 16:42

feat(common): state changed message

f389639

feat(common): state changed message

22b98f2

feat(common): state changed message

1276ac6

vercel bot had a problem deploying to Preview August 23, 2023 14:47 Failure

johannes-lindgren commented Aug 23, 2023

View reviewed changes

feat(common): state changed message

2f562ad

vercel bot had a problem deploying to Preview August 23, 2023 14:54 Failure

johannes-lindgren commented Aug 23, 2023

View reviewed changes

johannes-lindgren requested a review from eunjae-lee August 23, 2023 14:58

eunjae-lee reviewed Aug 23, 2023

View reviewed changes

eunjae-lee mentioned this pull request Aug 25, 2023

feat(lib): handle callbackId in contextRequest #261

Closed

feat(common): removing updater function

9c4adef

vercel bot had a problem deploying to Preview August 25, 2023 12:15 Failure

feat(common): removing default state

1647c87

vercel bot had a problem deploying to Preview August 25, 2023 12:44 Failure

feat(common): subscribing to state, resolving promised for loaded

d4e44c1

vercel bot had a problem deploying to Preview August 25, 2023 13:02 Failure

feat(common): removing setLoaded from FieldPluginActions.ts

e0f18aa

vercel bot had a problem deploying to Preview August 25, 2023 13:08 Failure

eunjae-lee reviewed Aug 25, 2023

View reviewed changes

vercel bot had a problem deploying to Preview September 4, 2023 13:38 Failure

fix type error

b597c70

vercel bot deployed to Preview September 4, 2023 13:52 View deployment

fix container to correctly keep isModalOpen state

ee4bbf6

eunjae-lee marked this pull request as ready for review September 4, 2023 15:01

eunjae-lee requested review from demetriusfeijoo and BibiSebi September 4, 2023 15:01

vercel bot deployed to Preview September 4, 2023 15:06 View deployment

This was referenced Sep 5, 2023

feat(lib): add parseContent #266

Merged

feat(lib): typed content #241

Closed

BibiSebi reviewed Sep 5, 2023

View reviewed changes

BibiSebi approved these changes Sep 5, 2023

View reviewed changes

demetriusfeijoo reviewed Sep 6, 2023

View reviewed changes

clean up comment and fix lint errors

89332f2

vercel bot deployed to Preview September 6, 2023 14:28 View deployment

eunjae-lee mentioned this pull request Sep 6, 2023

fix(helper): update react helper #267

Merged

demetriusfeijoo self-requested a review September 6, 2023 20:38

demetriusfeijoo approved these changes Sep 6, 2023

View reviewed changes

BibiSebi mentioned this pull request Sep 7, 2023

fix(helper): update vue3 helper #269

Merged

This was referenced Sep 7, 2023

fix(helper): update vue 2 template and helper #270

Merged

fix(demo): provide parseContent #271

Merged

eunjae-lee merged commit 9acda1f into main Sep 14, 2023

eunjae-lee deleted the EXT-1913-storyfront-send-a-new-event-updated-to-field-plugin-after-receiving-update-or-toggle-modal-event branch September 14, 2023 14:27

demetriusfeijoo mentioned this pull request Sep 23, 2024

feat(lib): add event listener for ESC key handler #404

Merged

		@@ -114,45 +96,42 @@ export const createPluginActions: CreatePluginActions = (

		return {
		actions: {

feat(common): new subscription model #260

feat(common): new subscription model #260

Conversation

johannes-lindgren commented Aug 23, 2023 • edited by eunjae-lee Loading

What?

Why?

How to test? (optional)

vercel bot commented Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

johannes-lindgren Aug 23, 2023 • 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

BibiSebi Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannes-lindgren commented Aug 23, 2023

eunjae-lee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eunjae-lee Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

eunjae-lee Aug 25, 2023 • 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

eunjae-lee Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

eunjae-lee commented Sep 4, 2023

eunjae-lee commented Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BibiSebi commented Sep 5, 2023

demetriusfeijoo 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

eunjae-lee commented Sep 6, 2023

demetriusfeijoo left a comment

Choose a reason for hiding this comment

eunjae-lee commented Sep 14, 2023 • edited Loading

Merge Activity

johannes-lindgren commented Aug 23, 2023 •

edited by eunjae-lee

Loading

vercel bot commented Aug 23, 2023 •

edited

Loading

johannes-lindgren Aug 23, 2023 •

edited

Loading

BibiSebi Aug 31, 2023 •

edited

Loading

eunjae-lee Aug 25, 2023 •

edited

Loading

eunjae-lee Aug 25, 2023 •

edited

Loading

eunjae-lee Aug 25, 2023 •

edited

Loading

eunjae-lee commented Sep 5, 2023 •

edited

Loading

eunjae-lee commented Sep 14, 2023 •

edited

Loading