Fix: #8914

[anton-pavlov-deel]

Description of change

Support async import for DataSource in CLI. Fixes #8914

Pull-Request Checklist

• Code is up-to-date with the master branch
• npm run format to apply prettier formatting
• npm run test passes with this change
• This pull request links relevant issues as Fixes #0000
• There are new or updated unit tests validating the change
• Documentation has been updated to reflect this change
• The new commits follow conventions explained in CONTRIBUTING.md

[pleerock]

can't we add a check (something like if (fileExport instanceof Promise)) in order to make it more clear? also I recommend to add a comment line and explain why this change is needed.

[anton-pavlov-deel]

@pleerock Sure, we can. It was decided to do so for sake of shortness (as far as await does not make (almost) any effect on non-Promise value this way).

But if it will be more clear this way, we can do like this (and the comment would be like that):

// It is necessary to await here in case of the exported async value (Promise<DataSource>).
// e.g. the DataSource is instantiated with an async factory in the source file
const awaitedFileExport = fileExport instanceof Promise 
  ? await fileExport
  : fileExport;

What do you think?

[Ginden]

@pleerock It's possible, but some people still use eg. Bluebird promises or compilation chains with Promise polyfills. There isn't any benefit in instanceof Promise checks, but only some obscure bug reports.

[V1raNi]

I think awaiting doesn't introduce any overhead, as MDN states - Returns the fulfilled value of the promise, or the value itself if it's not a Promise. And even if there is any, it is too small. So I would agree with @Ginden on this one

[pleerock]

yes so sometimes I prefer to have more code to make things more explicit / transparent and easier to understand.

[ViniciussSantos]

I have some questions about how commandUtils.loadDataSource works and how to test this new bugfix. First, I'm trying to create a test that works for the current version first before adapting it to work with the async version:

const dataSourceExports = []
for (let fileExport in dataSourceFileExports) {
    if (
        InstanceChecker.isDataSource(dataSourceFileExports[fileExport])
    ) {
        dataSourceExports.push(dataSourceFileExports[fileExport])
    }
}
  if (dataSourceExports.length === 0) {
            throw new Error(
                `Given data source file must contain export of a DataSource instance`,
            )
        }

But for some reason the InstanceChecker if statement is always false and the error is always thrown in my test and I can't seem to understand why.

Right now, my test involves a simple file exporting a dataSource instance like this:

const options: DataSourceOptions = {
    type: "postgres",
    host: "localhost",
    port: 5432,
    username: "test",
    password: "test",
    database: "test",
    synchronize: true,
    entities: [Post, Author, Category],
}
const dataSource = new DataSource(options)

module.exports = dataSource

and I'm testing it like this:

const DataSourcePath = __dirname + "/datasourceInstance"
        const response = await CommandUtils.loadDataSource(
            DataSourcePath,
        )
        expect(response).instanceOf(DataSource)

when I console.log dataSourceFileExports, I get the confirmation that it's actually a datasource instance:

but the instanceChecker never returns a true value. What am I doing wrong @Ginden @pleerock ?

[pleerock]

@ViniciussSantos not sure, you might need to debug and check the data source symbol values?

[jordanmnunez]

any update on pushing this through? Looks like it is really close

[texiontech]

when will release this issue.