-
Notifications
You must be signed in to change notification settings - Fork 7
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
Generate CRUD typeDefs and resolvers for a type that is annotated with @model
directive
#2
Conversation
…ional tests. Updated readme.
…e passed via context.
@model
directive@model
directive
@alexyuly This is ready for a review 👍 |
const context = { | ||
directives: { | ||
model: { | ||
store: new MongoStore({ connection: 'mongodb://localhost/my-database' }), |
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.
MongoStore
isn't imported in this example code.
README.md
Outdated
execute( | ||
schema, | ||
gql` | ||
mutate { |
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.
mutation
instead of mutate
?
|
||
export class ModelDirective extends SchemaDirectiveVisitor { | ||
public visitObject(type: GraphQLObjectType) { | ||
// TODO check that id field does not already exist on type |
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.
Or, you could require that the id
not be provided to types with a model directive.
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.
Right. The check will be to show an error to the user that an id
field already exists.
omitResolvers, | ||
} from './'; | ||
|
||
export const toInputObjectName = (name: string): string => `${name}InputType`; |
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.
It looks like you're creating only one input type per object type? How are you handling object create vs. update? What is the role of input types within your design?
In maxview-config, the reason I use separate inputs for create vs. update is that oftentimes, I want to update just a subset of an object's required fields. I don't want to have to provide all required fields to an update mutation, in order to update a subset of those fields.
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.
For each object type there is an identical input type with the same required fields.
You raise a good point about providing a subset of required fields for an updates. I need to think about this a bit more.
} | ||
|
||
type Mutation { | ||
_: Boolean |
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.
Lol @ graphql still requiring placeholder fields for an initial type declaration. Why doesn't graphql just let us declare multiple instances of a type and then merge them together? The extend
keyword has no useful purpose. Obviously it's not your problem, but it makes me laugh each time I see it.
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 guess there's a solution for it now? I have to investigate further.
|
||
return { | ||
...res, | ||
[key]: field, |
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.
Should you throw an error if the field is already set on the type, since this indicates that duplicate input field definitions exist, which may conflict? I don't think this should overwrite the existing field definition silently.
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.
Yep. I'm going to dedicate a new issue to error handling.
@@ -0,0 +1,13 @@ | |||
import { omitBy } from 'lodash'; | |||
|
|||
export const omitResolvers = (fields) => { |
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.
Can you avoid including resolvers in input types when they are created, so you don't have to omit them later? I bet you could improve this code. Adding resolvers only to omit them may not be the ideal code path?
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.
No, because the input type is based off of the object type whose fields have resolvers. In other words the fields of the object type are copied and become fields for an input type (with resolvers ommitted).
StoreUpdateReturn, | ||
} from 'graphql-model-directive'; | ||
import { cloneDeep } from 'lodash'; | ||
import mongoist from 'mongoist'; |
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.
Why mongoist
instead of mongodb
?
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.
Better async await support. Don't need to explicitly wait for the connection to be created, it handles that internally.
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.
Cool. I love it when APIs handle lifecycle operations internally, as they should.
const _id = mongoist.ObjectId(clonedObject.id); | ||
delete clonedObject.id; | ||
return { | ||
...clonedObject, |
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.
Why do you deeply clone object
, only to shallow clone it again? You don't need to deep clone it at all. Same goes for the other deep clone on line 68.
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.
Fixed.
const res = await this.db[props.type.name].update( | ||
this.formatInput(props.where), | ||
{ | ||
$set: props.data, |
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.
Does your design support deep updates of nested types?
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.
Yep, check this out in GraphiQL
type Item @model {
name: String
subItem: SubItem
}
type SubItem {
name: String
}
type Query {
_: Boolean
}
type Mutation {
_: Boolean
}
query items {
items {
id
name
subItem {
name
}
}
}
mutation createItem {
createItem(data: {
name:"item"
subItem: {
name: "sub item"
}
}) {
id
}
}
mutation updateItem {
updateItem(
where:{
name: "item"
}
data: {
name: "item",
subItem: {
name: "updated sub item"
}
})
}
query item {
item(where: {
subItem:{
name:"updated sub item"
}
}) {
id
name
subItem {
name
}
}
}
@model
directive which modifies schema to modify type and add query and mutation fields (see readme, the example or tests for in depth).@default
,@relation
,@unique
.The above example will generate the following schema with functioning resolvers.
Closes #7
Closes #9
Closes #13
Closes #6
Closes #15