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

Async support for build, buildList, create, createList #135

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jonstorer
Copy link
Contributor

@jonstorer jonstorer commented Mar 25, 2024

Hello,

I'm looking for feedback on my changes to add asynchronous support for the build and buildList methods. I've also added create and createList as well as lifecycle hooks beforeBuild, afterBuild (alias after), beforeCreate, onCreate, and afterCreate.

These changes maintain the existing api for synchronous execution but become asynchronous when an async hook is executed.

Example

Factory.define('Post', Post)
  .attr('title', () => faker.lorem.sentence())
  .attr('content', () => faker.lorem.paragraphs())
  .sequence('authorId')
  .attr('author', ['author'], (attrs = {}) => Factory.build('User', attrs))
  .option('commentCount', 2)
  .attr('comments', ['commentCount'], (count) => Factory.buildList('Comment', count))
  .beforeBuild((attributes, options) => {
    if (attributes.authorId) attributes.author = { id: attributes.authorId }
    else if (attributes.author) attributes.authorId = attributes.author.id
  })
  //.afterBuild(() => {}) // just showing these exist // '.after' calls '.afterBuild'
  //.beforeCreate(() => {}) // just showing these exist
  .onCreate(async (object, options) => {
    return await asyncPersist(object)
  })
  .afterCreate(async (record, options) => {
    return await asyncEagerLoadRelationships(record)
  })

Notes about these changes.

  • I will clean up my commits before merging, wip commits do not belong on the main trunk
  • I moved from jest to mocha + sinon because I know it better, I can move back.
  • the prettier / linter is disable, but will be enabled and resolved before merging
  • if things should be named differently, please let me know

@jonstorer
Copy link
Contributor Author

@eventualbuddha any feedback here?

@jonstorer
Copy link
Contributor Author

@bkeepers @mkuklis Hello! I'm hoping you can help me get @eventualbuddha's attention to look at this PR. And / or do either of you have feedback for this pr? And / or do you have write / publish rights to this repo?

Also, turns out I contributed to this repo ~10 years ago... Hopefully that buys me a little cred to get this change through. (Mind you, they weren't great contributions).

@eventualbuddha
Copy link
Contributor

To be honest, I don't use Rosie and haven't for some time. I'm not inclined to spend too much time on reviewing PRs and doing releases, but I would do more than zero. I don't really care about mocha vs jest, but I don't think that change or other build/test/lint changes should be mingled in with feature changes.

If you can make a feature PR that's focused and small I'd be up for reviewing it.

@mkuklis
Copy link
Contributor

mkuklis commented Apr 15, 2024

@jonstorer I only contributed minimally to this repo but I agree with @eventualbuddha. I would also keep all the lint rules unchanged (this seems unrelated to your change and it makes it much harder to review).

@jonstorer
Copy link
Contributor Author

@eventualbuddha & @mkuklis thank you for getting back to me. I will rework my changes back to jest and undo the linter changes. I'll nudge you again when it's ready for review.

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

Successfully merging this pull request may close these issues.

3 participants