mizdra/graphql-codegen-typescript-fabbrica

Change dynamic field interfaces

mizdra opened this issue · 5 comments

Motivation

  • The get function does not return a usable type.
    • The type of get('author') is Promise<OptionalAuthor | undefined>.
    • OptionalAuthor means { id?: string | undefined, name?: string | undefined, email?: string | undefined }.
    • All fields of author may be undefined. This is inconvenient for the user.
    • Therefore, get function is not very useful.
  • Dependent Field is not simple to define.
    • What it means...
      • Call get(...): get('name')
      • Add await keyword before get(...): await get('name')
      • Prepare default value: (await get('name')) ?? 'defaultName'
      • Compute the field: `${(await get('name')) ?? 'defaultName'}@yuyushiki.net`
      • Wrap with dynamic function: dynamic(async ({ get }) => `${(await get('name')) ?? 'defaultName'}@yuyushiki.net`)
    • Many steps are required. It is not simple.
    • I would like to define it more simply.

Proposal

Change the interface as follows:

Case. 1: Use seq in defaultFields and .build() function

Before

const BookFactory = defineBookFactory({
  defaultFields: {
    id: dynamic(({ seq }) => `Book-${seq}`),
    name: 'Book',
  },
});
const book = await BookFactory.build({
  name: dynamic(({ seq }) => `Book-${seq}`),
});

After

const BookFactory = defineBookFactory({
  defaultFields: (({ seq }) => ({
    id: `Book-${seq}`,
    name: 'Book',
  })),
});
const book = await BookFactory.build(({ seq }) => ({
  name: `Book-${seq}`,
}));

Case. 2: Use get for Dependent Fields

Before

const AuthorFactory = defineAuthorFactory({
  defaultFields: {
    name: 'mikamikomata',
    email: dynamic(async ({ get }) => `${(await get('name')) ?? 'defaultName'}@yuyushiki.net`),
  },
});

After

const AuthorFactory = defineAuthorFactory({
  defaultFields: (({ seq }) => {
    const name = 'mikamikomata';
    return {
      name,
      email: `${name}@yuyushiki.net`,
    };
  }),
});

Case. 3: Use get for Transient Fields (depend: #73)

Before

const AuthorFactory = defineAuthorFactory.withTransientFields({
  bookCount: 0,
})({
  defaultFields: {
    books: dynamic(async ({ get }) => {
      const bookCount = (await get('bookCount')) ?? 0;
      return BookFactory.buildList(bookCount);
    }),
  },
});

After

const AuthorFactory = defineAuthorFactory.withTransientFields({
  bookCount: 0,
})({
  defaultFields: async ({ transientFields: { bookCount } }) => ({
    books: await BookFactory.buildList(bookCount),
  }),
});

Breaking Changes

  • Remove get function
  • Remove dynamic utility

Please let me know if this should be a separate issue, but with the proposed changes: could the factories (maybe optionally) become synchronous? Looking at one example, since there's nothing async involved in defineXFactory I would assume that the value generated from .build() could also be synchronous:

const AuthorFactory = defineAuthorFactory({
  defaultFields: (({ seq }) => {
    const name = 'mikamikomata';
    return {
      name,
      email: `${name}@yuyushiki.net`,
    };
  }),
});

That might make it easier to reuse the factories in more places, e.g.: Storybook Stories where the args are declared synchronously.

@YellowKirby Thanks for your interest in this issue.

could the factories (maybe optionally) become synchronous?

What does factories mean? Do you mean AuthorFactory? Or do you mean .build() method?

What does factories mean? Do you mean AuthorFactory? Or do you mean .build() method?

Sorry, I meant the .build() (and .buildList()) method

OK.

However, making the build method synchronous is outside the scope of this issue. In this issue, the build method is asynchronous.

I understand the need for a synchronous build method in an environment where top-level await is not available. I have considered implementing that functionality in the past. But I gave up. I did not want to complicate the internal code.

Therefore, I recommend migrating to an ESM that allows for top-level await. Recently many tools support it these days. Storybook also support it (storybookjs/storybook#15129).

Okie dokie! That sounds reasonable, thank you! I think the top-level await will cover what I'm going for in Storybook, thanks for the link :)