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

feat(seeder): add seeder package #929

Merged
merged 1 commit into from
May 14, 2021
Merged

Conversation

Langstra
Copy link
Collaborator

@Langstra Langstra commented Oct 19, 2020

Currently this is still a WIP, but I am opening the PR already to gather some feedback. Nothing from this code has been tested yet and I am not even sure it will transpile or pass eslint and/or prettier.

todo:

  • Add documentation for the other CLI commands
  • Add configurable seeder class for --seed option for migration and schema fresh commands
  • Add configurable default seed class for fresh commands
  • Add documentation on the config option for the path for seed files
  • Refactor seed commands to seed:action
  • Add seed:create command for creating a seeder class

Closes #251

Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks! left some comments here, but in general without tests or docs its hard to suggest what to change how, so please add some tests or at least adjust the description with some example usage. I don't really like the global storage, I'd expect this to work similarly to migrations or entity generator - you as a user need to initialize the ORM and get the seed manager from that - that is where the context should be, no need to have anything globally.

package.json Outdated Show resolved Hide resolved
packages/seeder/package.json Outdated Show resolved Hide resolved
packages/seeder/src/factory.ts Outdated Show resolved Hide resolved
packages/seeder/src/factory.ts Outdated Show resolved Hide resolved
packages/seeder/src/factory.ts Outdated Show resolved Hide resolved
packages/seeder/src/refresh-database.decorator.ts Outdated Show resolved Hide resolved
packages/seeder/src/refresh-database.decorator.ts Outdated Show resolved Hide resolved
packages/seeder/src/refresh-database.decorator.ts Outdated Show resolved Hide resolved
packages/seeder/src/seeder.ts Outdated Show resolved Hide resolved
packages/seeder/src/utils/file.util.ts Outdated Show resolved Hide resolved
@B4nan
Copy link
Member

B4nan commented Nov 4, 2020

Do you plan to finalize this?

Copy link
Contributor

@merceyz merceyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have some comments on dependencies, staying out of the implementation part

packages/seeder/package.json Outdated Show resolved Hide resolved
packages/seeder/package.json Outdated Show resolved Hide resolved
packages/seeder/src/utils/file.util.ts Outdated Show resolved Hide resolved
@B4nan
Copy link
Member

B4nan commented Feb 9, 2021

Do you plan to continue working on this? I could pick it up myself, but I really need some docs (for the public api) and tests (or example usage) for what is done already.

@rubiin
Copy link
Contributor

rubiin commented Feb 11, 2021

i think it would be better https://github.com/CyriacBr/mikro-resources/tree/develop/packages/fixtures as its documeneted

@B4nan
Copy link
Member

B4nan commented Feb 11, 2021

@CyriacBr could you present a bit what's there and how it fits the #251?

I don't really want to add more decorators and drive it that way, the idea was to have something similar to laravel. Maybe your FixturesFactory is exactly that?

(sorry for dumb questions, never used any kind of seeder myself, same with laravel, so this issue is rather abstract to me)

@CyriacBr
Copy link
Contributor

@CyriacBr could you present a bit what's there and how it fits the #251?

I don't really want to add more decorators and drive it that way, the idea was to have something similar to laravel. Maybe your FixturesFactory is exactly that?

(sorry for dumb questions, never used any kind of seeder myself, same with laravel, so this issue is rather abstract to me)

Hello. I haven't used Laravel myself so I don't know how its Seeder behaves, but basically my package allows you to generate random entities on the fly using FixturesFactory. Thanks to MikroORM metadatas, decorators are only there for customization. (e.g if you do not want random values). I opted for decorators because it fits well within Typescript and MikroORM entities and everything is centralized.

It was written more for testing but can be used as a CLI by creating a .ts file and use the API there.

const orm = await MikroORM.init(ormConfig);
const factory = new FixturesFactory(ormInstance);
// Generate a fixture
const author = await factory.make(Author).oneAndPersist();
// Generate multiple fixtures
const authors = await factory.make(Author).manyAndPersist(10);

But note that I haven't used MikroORM in a while so I do not know if the package is up to date with the latest release.

@Langstra
Copy link
Collaborator Author

Yeah I am sorry for not working on this anymore.work took up a lot of time lately. I'll try and make some documentation and code for this in the coming weekends. I'll keep you posted.

@rubiin
Copy link
Contributor

rubiin commented Feb 13, 2021

Yeah I am sorry for not working on this anymore.work took up a lot of time lately. I'll try and make some documentation and code for this in the coming weekends. I'll keep you posted.
A documentation upto now would be great so that me or anyone else can pickup things where you left off

@Langstra
Copy link
Collaborator Author

I am still working on it and it is far from done, but I pushed my progress so you get a bit more feeling for and know I am working on it ;)

@Langstra Langstra force-pushed the seeder branch 4 times, most recently from 68beed8 to 1526365 Compare February 23, 2021 10:42
@Langstra
Copy link
Collaborator Author

I think right now would be a good moment for some feedback. In my opinion it is mostly done.

@Langstra Langstra force-pushed the seeder branch 5 times, most recently from 0fcc64f to dc1f4cd Compare February 25, 2021 10:29
@Langstra Langstra changed the title WIP: feat: seeder feat(seeder): add seeder package Feb 25, 2021
@Langstra Langstra requested a review from B4nan February 25, 2021 10:31
@B4nan
Copy link
Member

B4nan commented Feb 25, 2021

Thanks a lot, gave it a quick scan and it looks good! Will review the code in detail soon, and ideally we can get this out in v4.5

Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good, but i'd like to change the api to be sync as much as possible (while allowing to have async callbacks probably) to truly leverage the UoW

also the eval should go, i dont understand why it should be needed - seeders should be exported and we should require them as we do with entities or the orm config. dont worry about the eslint rule (add ignore comment for it) - this is the right place to do it.

docs/docs/seeding.md Outdated Show resolved Hide resolved
docs/docs/seeding.md Outdated Show resolved Hide resolved
docs/docs/seeding.md Outdated Show resolved Hide resolved
docs/docs/seeding.md Outdated Show resolved Hide resolved
docs/docs/seeding.md Outdated Show resolved Hide resolved
tests/cli/CLIHelper.test.ts Outdated Show resolved Hide resolved
tests/cli/DatabaseSeedCommand.test.ts Outdated Show resolved Hide resolved
packages/seeder/src/mikro-orm-seeder.ts Outdated Show resolved Hide resolved
packages/seeder/src/mikro-orm-seeder.ts Outdated Show resolved Hide resolved
packages/seeder/src/mikro-orm-seeder.ts Outdated Show resolved Hide resolved
@B4nan
Copy link
Member

B4nan commented Feb 26, 2021

anyway thanks a lot for this, especially for the docs. if you would struggle with the requests, testing or with time in general, this is already in a shape where i will be happy to take over myself. if so, i'd suggest to merge this to dev branch instead of master, so i can work with it directly.

@totomakers
Copy link

totomakers commented Feb 26, 2021

Seeder would be very nice \o/

I have made a proposal for Factory API here: https://github.com/totomakers/mikro-orm/tree/seeder-propose
Quite bit simpler with less type casting, let me know what you think about it :)

@Langstra Langstra changed the title feat(seeder): add seeder package Draft: feat(seeder): add seeder package Mar 3, 2021
@Langstra Langstra changed the title Draft: feat(seeder): add seeder package WIP: feat(seeder): add seeder package Mar 3, 2021
@B4nan
Copy link
Member

B4nan commented Mar 3, 2021

So what is it? draft? wip? or ready for review? :] I see you cracked the coverage 👍

One thing I can say right ahead - I don't like the seedString method, why should we have something like that? Also from the implementation you require default export, not cool as well. Looks like its not documented (so probably internal), and I kinda understand why its there (but the name is pretty bad 😉), will give it a bit more thinking...

Another quick note, it looks like you have the DatabaseSeeder name for the default seeder hardcoded in some places (handleFreshCommand, handleSchemaCommand), this should be probably configurable.

@Langstra
Copy link
Collaborator Author

Langstra commented Mar 3, 2021

Yeah, yesterday I thought I finished it, but then this morning I was like "oh shit this and this needs to be done".
So yeah, things I still need to work on:

  • Add documentation for the other CLI commands
  • Add configurable seeder class for --seed option for migration and schema fresh commands
  • Add configurable default seed class for fresh commands
  • Add documentation on the config option for the path for seed files
  • Refactor seed commands to seed:action
  • Add seed:create command for creating a seeder class

We need the seedString method for commands like npx database:seed --class=BookSeeder, because in the command BookSeeder is a string. So we load the seeder class file using require and then execute the seeder.

@RDeluxe
Copy link

RDeluxe commented Mar 3, 2021

Any chance you took a look at @totomakers proposal ?

@Langstra
Copy link
Collaborator Author

Langstra commented Mar 3, 2021

Any chance you took a look at @totomakers proposal ?

Yeah, I used some ideas from it, but I do not like loading all SeederClasses and running them I'd like to give the user the option to run whatever seeder he/she wants.

@B4nan
Copy link
Member

B4nan commented Mar 3, 2021

Yeah we want this "main seeder" that orchestrates child ones, having an array of classes is not that powerful - we need to control the flow.

@totomakers
Copy link

@Langstra i make some improvement in my company project. I can't share the whole project but i can let you take a peek.

@Langstra Langstra force-pushed the seeder branch 4 times, most recently from 6a8aae2 to bb65bfb Compare March 5, 2021 16:08
@lgtm-com
Copy link

lgtm-com bot commented Mar 5, 2021

This pull request introduces 1 alert when merging bb65bfb into 9517a1d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@Langstra
Copy link
Collaborator Author

Langstra commented Mar 5, 2021

One thing I can say right ahead - I don't like the seedString method, why should we have something like that? Also from the implementation you require default export, not cool as well. Looks like its not documented (so probably internal), and I kinda understand why its there (but the name is pretty bad ), will give it a bit more thinking...

Any suggestions for this? I believe this would be the last part of the feature to fix. Otherwise I am quite confident that the feature is ready.

@Langstra Langstra changed the title WIP: feat(seeder): add seeder package feat(seeder): add seeder package Mar 5, 2021
@B4nan
Copy link
Member

B4nan commented Mar 5, 2021

Don't worry too much about that, I am fine having it there, we should just pick a better name.

Will try to look into this in more detail during weekend.

This was referenced Mar 30, 2021
@B4nan
Copy link
Member

B4nan commented Apr 16, 2021

Time has come, please rebase this and let's merge it, master is now officially 5.x. I might tweak some things/naming directly there.

@Langstra
Copy link
Collaborator Author

Time has come, please rebase this and let's merge it, master is now officially 5.x. I might tweak some things/naming directly there.

Will work on it asap, won't have time for it this weekend, but I am aiming to fix it this week.

@B4nan
Copy link
Member

B4nan commented May 13, 2021

What about this? The rebase should be rather easy, right? :]

Or did I miss it and we have new conflicts? If so, sorry about that, I guess (force) pushing won't trigger any notifications...

@Langstra
Copy link
Collaborator Author

Yeah I already had, but now I have rebased it again. Should be fine and pass the pipeline again.

@B4nan B4nan merged commit 2b86e22 into mikro-orm:master May 14, 2021
@rubiin
Copy link
Contributor

rubiin commented May 14, 2021

cool waiting for new version soon to use this

@edorgeville
Copy link

Great job @Langstra !

@Langstra Langstra deleted the seeder branch January 3, 2022 18:54
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.

Create seeds as in Eloquent (Laravel)
9 participants