-
Notifications
You must be signed in to change notification settings - Fork 1
feat(schema): Use Neo4j DateTime type for created and modfied fields #154
Conversation
Looks good. If we set the created and updated properties automatically, nobody would need to update or set these properties manually. We might even remove them from the update/create mutations. |
Yes, I like the idea of removing them from the mutations. |
fieldNode.arguments.push(buildArgument({ name: buildName({ name: 'created' }), value })) | ||
} | ||
|
||
if (action === 'Create' || action === 'Update' || action === 'Update') { |
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 action == Update twice?
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.
sorry, just saw the other commit that changed it to Merge
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.
Yes, that was wrong. Fixed it right after.
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 can't approve my own PR! but it looks good by me
} | ||
|
||
if (action === 'Create') { | ||
fieldNode.arguments.push(buildArgument({ name: buildName({ name: 'created' }), value })) |
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 just had a thought here: What if the type has no 'created' or 'modified' fields? Here you will try and add them anyway.
All of the existing models implement ThingInterface and so have these fields, but some of the types that I defined for supporting the Annotations schema don't include them. I can add them in, but I think it makes sense that this transformer also supports the case where they're not present
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.
Oh, that's a very good point. I'd assumed every type would contain these properties...
This was originally set to String, but it's nice to have it as a datatype that allows us to do more detailed filtering.
This isn't affected by #91 because we will always have a full datetime for created and modified.
An addition to this is to integrate #112 at a later stage.
One issue that I saw, I can perform this mutation (adding created.formatted):
but this one fails:
with this message:
@ChristiaanScheermeijer thinks it's related to neo4j-graphql/neo4j-graphql-js#547