Fix: #8914 #8917
Fix: #8914 #8917
Conversation
src/commands/CommandUtils.ts
Outdated
| ) { | ||
| dataSourceExports.push(dataSourceFileExports[fileExport]) | ||
| for (const fileExport of dataSourceFileExports) { | ||
| const awaitedFileExport = await fileExport |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
yes so sometimes I prefer to have more code to make things more explicit / transparent and easier to understand.
|
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 = dataSourceand 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: |
|
@ViniciussSantos not sure, you might need to debug and check the data source symbol values? |
|
any update on pushing this through? Looks like it is really close |
|
when will release this issue. |
…#8917) * Fix: await DataSource from export file to support async loading * fix: prettier errors * updated code style Co-authored-by: Umed Khudoiberdiev <[email protected]>
anton-pavlov-personal
deleted the
fix/8914-cli-does-not-support-async-datasource
branch
…#8917) * Fix: await DataSource from export file to support async loading * fix: prettier errors * updated code style Co-authored-by: Umed Khudoiberdiev <[email protected]>
8 participants
Description of change
Support async import for DataSource in CLI. Fixes #8914
Pull-Request Checklist
masterbranchnpm run formatto apply prettier formattingnpm run testpasses with this changeFixes #0000