storyblok / field-plugin

Create and deploy Storyblok Field Plugin
https://www.storyblok.com/docs/plugins/field-plugins/introduction
25 stars 3 forks source link

feat(lib): handle `callbackId` in `selectAsset` #253

Closed eunjae-lee closed 1 year ago

eunjae-lee commented 1 year ago

What?

The corresponding pull request in Storyfront is a prerequisite: https://github.com/storyblok/storyfront/pull/4368

This PR makes the following changes

Okay, but why ignore it instead of rejecting the promise?

Let's assume there is a field plugin that has called createFieldPlugin() twice.

// Comp1.jsx
const plugin1 = createFieldPlugin();
const result = await plugin1.actions.selectAsset();

// Comp2.jsx
const plugin2 = createFieldPlugin();
const result = await plugin2.actions.selectAsset();

Technically selectAsset can be called simultaneously because the asset selector will pop up as a modal and user won't be able to trigger selectAsset at the same time. However, let's assume that's somehow done programmatically.

The asset selector modal is up by plugin1, and user selects an asset. Now the message is delivered via postMessage. Both plugin1 and plugin2 will receive this message, because both plugins are listening to message event. By comparing the callbackId, we know it's originated from plugin1. So we execute the callback for plugin1. However, what about plugin2? It may have started its own asset selection process in the meantime, so we shouldn't already reject its callback yet. Let it stall. That's why the unit test doesn't check if it gets rejected, but it checks it's neither resolved nor rejected after some time out.

JIRA: EXT-1916

How to test? (optional)

vercel[bot] commented 1 year ago

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 Aug 22, 2023 3:24pm
demetriusfeijoo commented 1 year ago

Nice job @eunjae-lee, thanks.

I added some ideas but nothing is required to change.

Is there a need for updating the 'demo' application too, though?

eunjae-lee commented 1 year ago

Nice job @eunjae-lee, thanks.

I added some ideas but nothing is required to change.

Is there a need for updating the 'demo' application too, though?

oh we actually need to update the container just like I did to Storyfront: https://github.com/storyblok/storyfront/pull/4368

demetriusfeijoo commented 1 year ago

The changes look good to me @eunjae-lee. Nice job 🚀