storyblok / field-plugin

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

Extending `data.content` without losing type-safety #201

Closed marsidev closed 9 months ago

marsidev commented 1 year ago

Is your feature request related to a problem? Please describe. Hey! I know this project is in alpha stage, but I've been using it to create some plugins and it works like a charm, much better than the legacy method. Great work!

Since the templates are written in TypeScript, would be nice to have generic types, specially to customize the useFieldPlugin().data.content (which is unknown) without losing type-safety. In your templates you treat the content as number to store the value of the counter. In my case, I would like to store an object instead.

What I've done to achieve that is modifying the FieldPluginProvider.vue (vue3 template) and useFieldPlugin.ts to inject my custom types. By doing this, I have safe autocompletion in useFieldPlugin().data.content and useFieldPlugin().data.setContent().

image

I made this codesandbox to show my workaround, based on the vue3 template.

Files of interest: src/types.ts src/useFieldPlugin.ts src/components/FieldPluginProvider.vue src/components/Counter.vue src/components/Foo.vue

Describe the solution you'd like If you see src/types.ts, I added an interface (PluginContent) for my custom content, and rewrite FieldPluginData, FieldPluginActions and FieldPluginResponse to include the PluginContent. Also I updated some pieces in the createFieldPlugin callback function used in FieldPluginProvider.vue.

Instead rewriting those types and monkey-patching that callback would be nicer to do something like:

const plugin = reactive<PluginResponse<MyContent>>({
  type: 'loading',
});

createFieldPlugin<MyContent>(() => {})

Not sure that would be the best solution, it's just an idea.

Describe alternatives you've considered Nothing else

Additional context Nothing else

johannes-lindgren commented 1 year ago

Hi @marsidev thank you so much for your kind words and your feedback!

We discussed this in the team earlier this week, and we may soon have a plan for how to introduce increased type safety.

The reason why the type of data.content is unknown is that there is no guarantee that the content that the app receives from the visual editor adheres to the expected format. For example:

The way to deal with this in your example is to validate the data before it's being used. For example

const content = typeof data.content === 'number' ? data.content : 0
// use `content` instead of data.content 

or

const parseContent = (content: unknown, default: number): number =>  typeof content === 'number' ? content : default
const content = parseContent(data.content, 0)
// use `content` instead of data.content 

This works quite well for most use cases, though it gets more complicated when you're using an updater function

setContent((oldContent) => {
   // `oldContent` is `unknown` again :( 
})

So we're thinking of adding a parameter to createFieldPlugin

createFieldPlugin<T>(onResponse, parseContent: (content: unknown) => T)

Which would allow us to guarantee the correct type of data.content and the content argument in the actions.setContent updater function.

marsidev commented 1 year ago

Hey @johannes-lindgren,

Thanks for the update. Glad to read that you guys are working on an increased type safety.

After reading, it makes sense to have data.content as unknown. The generic and the parser argument may add type safety at least in the IDE scope.

I wonder if it would work also for objects instead of a plain value (for a plugin with multiple fields). Let's say data.content.name and data.content.age, with type safety and autocompletion while reading:

const content = parseContent(data.content, initialValues)
const name = content.name // no TS error
const age = content.age // no TS error

And also while updating:

const currentData = parseContent(data.content, initialValues)
actions.setContent({
  ...currentData,
  // or ...data.content as MyContent,
  name: 'Johannes' // no TS error
});
johannes-lindgren commented 1 year ago

Yes, I used number in my example above just because it was the simplest case to show, but the real motivation behind this proposal is the usage with more complex data types. parseData (or something equivalent) would be optional and provided by the user.

See this example where I'm using Zod to parse an object. The idea is to provide such a function (validation + fallback) to useFieldPlugin, which in turn provides it to createFieldPlugin, which results in strong typing for data.content and actions.setContent. That would enable

actions.setContent((oldContent) => ({
  ...oldContent, // strongly typed
  name: 'Johannes'
}));
marsidev commented 1 year ago

Awesome! Looking forward for such release. ✨

alvarosabu commented 1 year ago

@johannes-lindgren I was about to create a ticket similar to this, have you considered setting the content property as Record<string, unknow>?

eunjae-lee commented 1 year ago

@johannes-lindgren has internally proposed a new option to the function which is like

createFieldPlugin<TContent>({
  parseContent?: (content: unknown) => TContent
})

where you optionally write parseContent to make sure the content is in a right format, and returns it with correct type.

What do you think @alvarosabu ? I'd like to hear what you think. Any other ideas are welcome.

alvarosabu commented 1 year ago

Hi @eunjae-lee you gave me an idea actually

What about using generics?

// useFieldPlugin.ts
const plugin = inject<FieldPluginResponse<TContent>>()
// FieldPluginResponse.d.ts
export type FieldPluginResponse<T> = {
    type: 'loading';
    error?: never;
    data?: never;
    actions?: never;
} | {
    type: 'error';
    error: Error;
    data?: never;
    actions?: never;
} | {
    type: 'loaded';
    error?: never;
    data: FieldPluginData<T>;
    actions: FieldPluginActions;
};

And then


export type FieldPluginData<T = unknown> = {
    isModalOpen: boolean;
    content: T;
    options: Record<string, string>;
    spaceId: number | undefined;
    story: StoryData;
    storyId: number | undefined;
    blockUid: string | undefined;
    token: string | undefined;
    uid: string;
};

With this, you allow the user to set the type of content without having to use a parseContent method, you just pass the type down like you would do with an Array<T> or any other generic

eunjae-lee commented 1 year ago

@alvarosabu that'd be ideal, but there is one problem. When you're inside the Visual Editor, by default, it injects an empty string to field plugins as initial values. For example, you create a new story, and your field plugin has an empty string as content by default. With the generic approach above, the code will think it's type T but actually it isn't 🥲

demetriusfeijoo commented 9 months ago

Hey @johannes-lindgren @eunjae-lee, I think we can close this one as it was done here, right? Or is there something else to still be done?

eunjae-lee commented 9 months ago

@demetriusfeijoo It's correct. As long as you provide validateContent, the type of Content is inferred.

  createFieldPlugin({
    validateContent: (content: unknown) => {
      if (typeof content === 'string') {
        return {
          content,
        }
      } else {
        return {
          content,
          error: `content is expected to be a string (actual value: ${JSON.stringify(content)})`,
        }
      }
    }
  })

or

  useFieldPlugin({
    validateContent: (content: unknown) => {
      if (typeof content === 'string') {
        return {
          content,
        }
      } else {
        return {
          content,
          error: `content is expected to be a string (actual value: ${JSON.stringify(content)})`,
        }
      }
    }
  })
marsidev commented 9 months ago

Great work. Thank you!