Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change dynamic field interfaces #74

Open
mizdra opened this issue Mar 17, 2024 · 5 comments
Open

Change dynamic field interfaces #74

mizdra opened this issue Mar 17, 2024 · 5 comments
Labels
Type: Change Change existing functionality.

Comments

@mizdra
Copy link
Owner

mizdra commented Mar 17, 2024

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
@YellowKirby
Copy link

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.

@mizdra
Copy link
Owner Author

mizdra commented Apr 12, 2024

@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?

@YellowKirby
Copy link

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

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

@mizdra
Copy link
Owner Author

mizdra commented Apr 13, 2024

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).

@YellowKirby
Copy link

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Change Change existing functionality.
Projects
None yet
Development

No branches or pull requests

2 participants