-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add support for AdminBro v1.4 #2
Conversation
@@ -10,9 +10,13 @@ The plugin can be registered using standard `AdminBro.registerAdapter` method. | |||
|
|||
```typescript | |||
import { Database, Resource } from "admin-bro-typeorm"; | |||
const AdminBro = require('admin-bro'); | |||
import AdminBro from 'admin-bro' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import works with admin-bro@1.4.0
organization: Organization; | ||
|
||
// in order be able to fetch resources in admin-bro - we have to have id available | ||
@RelationId((person: Person) => person.organization) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find a better solution for relations
@@ -0,0 +1,18 @@ | |||
module.exports = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is needed for automated tests
@@ -0,0 +1,52 @@ | |||
import { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all models are needed just for automated tests
|
||
return resources; | ||
} | ||
|
||
public static isAdapterFor(database: any) | ||
{ | ||
return database instanceof Connection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some issues with instanceonf
with other adapters. Namely, when you are running app in the dev environment all devDependencies
are installed, and an entire application has different versions of typeorm package and instanceof doesn't work. On production typeorm is installed as a peerDependency (only one version exists) and then it works.
That is why I changed that to checking if passed object has entityMetadatas
property
private model: typeof BaseEntity; | ||
private propsObject: Record<string, Property> = {}; | ||
private propsArray: Array<Property> = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use object.values to fetch an actual array
@@ -89,36 +90,37 @@ export class Resource extends (BaseResource as any) | |||
[ sortBy ]: (direction || "asc").toUpperCase() | |||
} | |||
}); | |||
return instances.map(instance => new ExtendedRecord(instance, this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed extended record - because I think we don't need it. Or we need it?
} | ||
return params; | ||
} | ||
|
||
private async validateAndSave(instance: BaseEntity) { | ||
if (Resource.validate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is quite a hack - class-validator has to be passed to dev dependency because we are using it on tests. But when we pass it like that in the dev mode there are 2 class-validator packages active. The one from the original project and one from the admin-bro-typeorm, and this doesn't work. The way I handled that is to inject the class-validator via Resource.validate - checkout example in the readme.
This is temporary handling validations in typeorm. Long term would be to add something like BaseValidator in AdminBro, but I don't even think how to handle that right now.
@@ -64,7 +66,7 @@ extend(NUMBER, "number"); | |||
extend(STRING, "string"); | |||
extend(DATE, "datetime"); | |||
extend(BOOLEAN, "boolean"); | |||
extend(ARRAY, "array"); | |||
extend(OBJECT, "object"); | |||
// extend(ARRAY, "array"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no array type right now - instead there is an isArray property, but I didn't cover arrays at all in this PR - maybe in some time we will make an addition
@@ -11,7 +11,9 @@ | |||
"esModuleInterop": false, | |||
"declaration": true, | |||
"declarationDir": "./lib", | |||
"outDir": "./lib" | |||
"outDir": "./lib", | |||
"emitDecoratorMetadata": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed by typeorm (in automated test)
Hi,
I've just updated admin to version 1.4 with better TypeScript support (the thing you mentioned in one of the issues), so it should work just fine.
We have one new project which uses typeorm so took the liberty of forking your repo and adding a couple of things. Right now we are testing them on the project, but I wanted to know your opinion.
I will try to describe them in the comments below