fix(lib): include userid to data response from field plugin #438

ECJ222 · 2024-11-29T12:33:30Z

What?

This PR includes the userID to the data response retrieved on state change and loaded state from a field plugin as documented within our Field plugin API documentation

Why?

JIRA: SHAPE-7741

How to test? (optional)

To test, run the following commands below (on separate terminals):

yarn dev:lib.
yarn dev:demo.
yarn dev:sandbox.

These commands above should get your sandbox, field-plugin demo, and field-plugin library setup and running successfully.

Copy your currently running local sandbox URL, http://localhost:7070/, and place it within the file packages/field-plugin/src/createFieldPlugin/createFieldPlugin.ts.

This facilitates communication between your sandbox and field plugin through the Iframe when interacting with the rendered Iframe window.

fix.SHAPE-7741.mov

You should find the userId property returned and other plugin-related data from your field plugin demo.

vercel · 2024-11-29T12:33:34Z

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	Dec 4, 2024 9:09am

johannes-lindgren

Hi @ECJ222, thank you for your contribution; this will be a nice addition!

This is what we need to to move forward:

Update the validation functions. This does not show up as typescript errors because is essentially asserts the type. (And corresponding unit tests)
We need the "how to test" section to be filled out (it says that it's optional, but we need it in this instance). We can use the demo and sandbox for this.

After this PR, we will need to follow up with more PRs to perform the release.

johannes-lindgren · 2024-12-03T11:26:09Z

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

@@ -27,7 +27,7 @@ export type LoadedMessage = MessageToPlugin<'loaded'> & {
 }

 // TODO full implementation of validation
-export const isLoadedMessage = (data: unknown): data is LoadedMessage =>
+export const isLoadedMessage = (data: unknown): boolean =>


This is going to disable the type narrowing

This probably explains my warning below 🤔

I'm not sure the warnings you got from the visual editor were from this PR 🙏

They are no longer appearing after the last commit 🙏

Dawntraoz

Something is broken, I tested the demo with the changes you made by building it and adding the dist/index.js to My Plugins using the storyfront branch with the changes, and I got this warning while the plugin never loads:

The not loading wasn't related to the warnings, I managed to make it work on the Visual Editor 🎉 So let's fix those warnings and we are ready to go 🚀

ECJ222 · 2024-12-03T13:07:12Z

Awesome! @Dawntraoz 🫶

Was this the error you were talking about?

Uncaught Error: This error can be safely ignored. It is caused by the legacy field plugin API. See issue

Dawntraoz · 2024-12-03T13:13:02Z

Awesome! @Dawntraoz 🫶

Was this the error you were talking about?
Uncaught Error: This error can be safely ignored. It is caused by the legacy field plugin API. See issue

Oh no, that error is always there on purpose.

I meant the warning messages:
[Vue warn]: Invalid prop: type check failed for prop "spaceId". Expected Number with value X, got String with value "X"

Dawntraoz · 2024-12-03T13:17:51Z

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

@@ -33,6 +34,8 @@ export const isStateMessage = (data: unknown): data is StateChangedMessage =>
  typeof data.language === 'string' &&
  hasKey(data, 'schema') &&
  isFieldPluginSchema(data.schema) &&
+  hasKey(data, 'userId') &&


Thanks for this @ECJ222 🫂

I meant the warning messages:
[Vue warn]: Invalid prop: type check failed for prop "spaceId". Expected Number with value X, got String with value "X"

Oh, I don't think it's related to this PR.

johannes-lindgren · 2024-12-03T14:04:21Z

@ECJ222 I think there's a bug in the tests: it's passing the userId and spaceId as strings:

It worked before, because the validation function actually does not perform a check on the spaceId property (see the TODO comment)

ECJ222 · 2024-12-03T14:20:59Z

Thanks man, I updated it!

ECJ222 · 2024-12-03T14:25:55Z

Once this PR gets merged, I will follow up with the release PR. 🙏

packages/lib-helpers/test/src/index.ts

Dawntraoz

The code looks perfect to me! 🎉

Great job @ECJ222, thanks a lot for helping us with this 💙 And thanks to @johannes-lindgren for encouraging testing and validation and helped us review the PR 🫂

johannes-lindgren

Perfect, well done!

I checked that it works:

johannes-lindgren · 2024-12-04T10:50:39Z

...ield-plugin/src/messaging/pluginMessage/containerToPluginMessage/StateChangedMessage.test.ts

+    })
+  })
+
+  describe('the "language" property', () => {


johannes-lindgren · 2024-12-04T12:16:54Z

@Dawntraoz, @demetriusfeijoo, and @ECJ222: regarding the validation: when I wrote the library ~1.5 years ago, I did not include a validation library for two reasons:

I could not find a library of sufficiently small size
I could not find a library that did not infer the types from the schema. Zod (and other libraries) are nice and all, but if you want typescript support, you need to infer the types from the validation libraries. But this means that the users of the @storyblok/field-plugin will get Zod types when they're using the library, and the types will not as readable as when you type them yourself.

However, with a validation library, the problem that I requested changes for earlier would not have happened.

We also have this dilemma in the app, which is why I created a new validation library to fit these needs (amongst others): PureParse. Check it out if you're interested 🙂

Dawntraoz · 2024-12-04T12:43:32Z

@Dawntraoz, @demetriusfeijoo, and @ECJ222: regarding the validation: when I wrote the library ~1.5 years ago, I did not include a validation library for two reasons:

I could not find a library of sufficiently small size

I could not find a library that did not infer the types from the schema. Zod (and other libraries) are nice and all, but if you want typescript support, you need to infer the types from the validation libraries. But this means that the users of the @storyblok/field-plugin will get Zod types when they're using the library, and the types will not as readable as when you type them yourself.

But without a validation library, the problem that I requested changes for earlier would not have happened.

We also have this dilemma in the app, which is why I created a new validation library to fit these needs (amongst others): PureParse. Check it out if you're interested 🙂

This conversation is interesting 🤔 Maybe @demetriusfeijoo has more insights here, but since I joined the Plugins Team, we have been using https://valibot.dev/ in all our plugins since it seemed perfect for our use cases at the moment. So maybe we can use it here, too, to keep it consistent between all the projects we have and not use a different one per project 🤔

ECJ222 requested review from demetriusfeijoo and Dawntraoz November 29, 2024 12:33

vercel bot deployed to Preview November 29, 2024 12:34 View deployment

ECJ222 force-pushed the fix/SHAPE-7741-include-userid-in-fieldplugin-response-data branch from 443ef54 to 4439855 Compare November 29, 2024 12:38

ECJ222 changed the title ~~fix(SHAPE-7741): include userid to data response from field plugin~~ fix(lib): include userid to data response from field plugin Nov 29, 2024

vercel bot deployed to Preview November 29, 2024 12:49 View deployment

ECJ222 requested a review from johannes-lindgren November 29, 2024 13:14

fix(SHAPE-7741): include userid in field plugin response data

604a79c

ECJ222 force-pushed the fix/SHAPE-7741-include-userid-in-fieldplugin-response-data branch from 4439855 to 604a79c Compare December 2, 2024 16:23

vercel bot deployed to Preview December 2, 2024 16:27 View deployment

johannes-lindgren requested changes Dec 2, 2024

View reviewed changes

ECJ222 requested a review from johannes-lindgren December 3, 2024 08:14

vercel bot deployed to Preview December 3, 2024 08:15 View deployment

Dawntraoz assigned ECJ222 Dec 3, 2024

johannes-lindgren requested changes Dec 3, 2024

View reviewed changes

Dawntraoz requested changes Dec 3, 2024

View reviewed changes

chore(SHAPE-7741): update validation and unit test

34df2be

ECJ222 force-pushed the fix/SHAPE-7741-include-userid-in-fieldplugin-response-data branch from 7cb28de to 34df2be Compare December 3, 2024 13:07

vercel bot deployed to Preview December 3, 2024 13:09 View deployment

Dawntraoz reviewed Dec 3, 2024

View reviewed changes

chore(SHAPE-7741): update test user id

217f39a

vercel bot deployed to Preview December 3, 2024 14:21 View deployment

ECJ222 requested review from Dawntraoz and johannes-lindgren December 3, 2024 14:21

Dawntraoz reviewed Dec 3, 2024

View reviewed changes

packages/lib-helpers/test/src/index.ts Outdated Show resolved Hide resolved

chore(SHAPE-7741): remove console statement

0ba86bc

ECJ222 requested a review from Dawntraoz December 4, 2024 09:09

vercel bot deployed to Preview December 4, 2024 09:09 View deployment

Dawntraoz approved these changes Dec 4, 2024

View reviewed changes

johannes-lindgren approved these changes Dec 4, 2024

View reviewed changes

ECJ222 merged commit 629e6f4 into main Dec 4, 2024
4 checks passed

ECJ222 deleted the fix/SHAPE-7741-include-userid-in-fieldplugin-response-data branch December 4, 2024 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(lib): include userid to data response from field plugin #438

fix(lib): include userid to data response from field plugin #438

ECJ222 commented Nov 29, 2024 •

edited

Loading

vercel bot commented Nov 29, 2024 •

edited

Loading

johannes-lindgren left a comment

johannes-lindgren Dec 3, 2024

Dawntraoz Dec 3, 2024

ECJ222 Dec 3, 2024

Dawntraoz Dec 3, 2024

Dawntraoz left a comment •

edited

Loading

ECJ222 commented Dec 3, 2024

Dawntraoz commented Dec 3, 2024

Dawntraoz Dec 3, 2024

ECJ222 Dec 3, 2024

johannes-lindgren commented Dec 3, 2024

ECJ222 commented Dec 3, 2024

ECJ222 commented Dec 3, 2024

Dawntraoz left a comment •

edited

Loading

johannes-lindgren left a comment

johannes-lindgren Dec 4, 2024

johannes-lindgren commented Dec 4, 2024 •

edited

Loading

Dawntraoz commented Dec 4, 2024

fix(lib): include userid to data response from field plugin #438

fix(lib): include userid to data response from field plugin #438

Conversation

ECJ222 commented Nov 29, 2024 • edited Loading

What?

Why?

How to test? (optional)

vercel bot commented Nov 29, 2024 • edited Loading

johannes-lindgren left a comment

Choose a reason for hiding this comment

johannes-lindgren Dec 3, 2024

Choose a reason for hiding this comment

Dawntraoz Dec 3, 2024

Choose a reason for hiding this comment

ECJ222 Dec 3, 2024

Choose a reason for hiding this comment

Dawntraoz Dec 3, 2024

Choose a reason for hiding this comment

Dawntraoz left a comment • edited Loading

Choose a reason for hiding this comment

ECJ222 commented Dec 3, 2024

Dawntraoz commented Dec 3, 2024

Dawntraoz Dec 3, 2024

Choose a reason for hiding this comment

ECJ222 Dec 3, 2024

Choose a reason for hiding this comment

johannes-lindgren commented Dec 3, 2024

ECJ222 commented Dec 3, 2024

ECJ222 commented Dec 3, 2024

Dawntraoz left a comment • edited Loading

Choose a reason for hiding this comment

johannes-lindgren left a comment

Choose a reason for hiding this comment

johannes-lindgren Dec 4, 2024

Choose a reason for hiding this comment

johannes-lindgren commented Dec 4, 2024 • edited Loading

Dawntraoz commented Dec 4, 2024

ECJ222 commented Nov 29, 2024 •

edited

Loading

vercel bot commented Nov 29, 2024 •

edited

Loading

Dawntraoz left a comment •

edited

Loading

Dawntraoz left a comment •

edited

Loading

johannes-lindgren commented Dec 4, 2024 •

edited

Loading