8000 Async support for build, buildList, create, createList by jonstorer · Pull Request #135 · rosiejs/rosie · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

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.

Sign up for GitHub

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

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

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

wants to merge 5 commits into from
+4,126 −4,849

Conversation

jonstorer
Copy link
Contributor
@jonstorer jonstorer commented Mar 25, 2024 •
edited
Loading

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 jonstorer mentioned this pull request Mar 25, 2024
Closed
@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.

@jonstorer
Copy link
Contributor Author

@eventualbuddha @mkuklis I'm finally getting back to this. The plan is:

  • uplift current master tests with test dependencies that do not need a browser
  • uplift current master lint & prettier
  • bring new async capabilities into master

@jonstorer jonstorer mentioned this pull request Apr 1, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jonstorer @eventualbuddha @mkuklis
0