[bug] false-positive success when using named export on seeder file

[micalevisk]

Basically, when the seeder file has some export const foo = 'bar'-ish statement, the run method is not invoked and the output is this:

🌱  TypeORM Seeding v1.6.1
✔ ORM Config loaded
✔ Factories are imported
✔ Seeders are imported
⠋ Connecting to the databaseINFO: All classes found using provided glob pattern xxx
INFO: All classes found using provided glob pattern xxx
✔ Database connected
👍  Finished Seeding

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]]

typeorm-seeding/src/importer.ts

Lines 3 to 7 in 4136b0e

	export const importSeed = async (filePath: string): Promise<SeederConstructor> => {
	const seedFileObject: { [key: string]: SeederConstructor } = await import(filePath)
	const keys = Object.keys(seedFileObject)
	return seedFileObject[keys[0]]
	}

because this will return the foo variable instead of the seeder class. I've thought that return seedFileObject.default was enough but @hirsch88 already use this in the past with require (on 84df811)

Maybe this should work but I'm not sure:

export const importSeed = async (filePath: string): Promise<SeederConstructor> => {
  const seedFileObject: { [key: string]: SeederConstructor } = await import(filePath)
  return seedFileObject.default
}

[jorgebodega]

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

[micalevisk]

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

[jorgebodega]

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 🙏

[micalevisk]

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?

[jorgebodega]

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!

[RaphaelWoude]

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.

[jorgebodega]

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.

typeorm-seeding/src/types.ts

Lines 23 to 28 in 4136b0e

	/**
	* Seed are the class to create some data. Those seed are run by the cli.
	*/
	export interface Seeder {
	run(factory: Factory, connection: Connection): Promise<void>
	}

[micalevisk]

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

[jorgebodega]

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.

[micalevisk]

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.