Open micalevisk opened 3 years ago
Hi! Sorry for late response. I think it could be better to detect the seeder by some kind or regexp or maybe by using file name.
Feel free to dissent with me, open to more ideas
To me, using export default
to identify the seeder class is pretty good. But I don't know what are the odds of relying on default
property
I have no problem to use default export, but usually I try to avoid that. This link explain very well why.
But what I said, I have no problem with this if we are consistent, but could be a breaking change for someone that is using non-default exports.
@RaphaelWoude opinions here, pls :pray:
This link explain very well why.
@jorgebodega got it.
For now, you could add some note on README regarding this limitation. Something like: "Seeder files must not contain any other export
statement besides the one that exports the seeder class (due to this issue)" right?
Yes, that could work temporary while we still work on this. I will keep this issue open to use it as a reference.
Thanks for your help!
I think we should just use the modern module syntax. I have had a ton of problems before with export default. The link you provided pretty much sums it up.
For me, the best way to do this is by using non default export and use instanceof
to check every import as a Seeder, that is an interface.
then this interface becomes
abstract class Seeder {
abstract run(factory: any, connection: any): Promise<void>
}
and now users should move class Foo implements Seeder
to class Foo extends Seeder
? This approach looks good to me. This could be a problem if someone has been using
Class Foo implements Seeder extends Bar
tho, since they cannot extends from both Seeder
and Bar
For me mikro-orm seeding is a very good example on how to do this. But is unreleased yet.
I'm pretty sure we should create a project and start working on next version, maybe a 2.x version, because there will be breaking changes for sure.
cool!
What if there are multiple seeder class exported? I don't know how Mikro ORM handle this. My suggestion is: to prevent misuages, typeorm-seeding
will throw an error if more than one seeder class is found.
Basically, when the seeder file has some
export const foo = 'bar'
-ish statement, therun
method is not invoked and the output is this:I think that typeorm-seeding CLI should ignore these statements regardless of whether it is right or not to have them in a seeder file.
I've notice that this behavior is due to
seedFileObject[keys[0]]
https://github.com/w3tecch/typeorm-seeding/blob/4136b0e0f58a4ec13b17c717bd026043fbdf7ddd/src/importer.ts#L3-L7
because this will return the
foo
variable instead of the seeder class. I've thought thatreturn seedFileObject.default
was enough but @hirsch88 already use this in the past withrequire
(on 84df811)Maybe this should work but I'm not sure: