-
-
Notifications
You must be signed in to change notification settings - Fork 55
Add support for null property values #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and co 8000 ntact 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
Changes from all commits
eeeef5a
e64bdef
9660d7d
bf3afb0
3f3614d
4a75bb6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import { | |
import { PrimaryKey } from '../primaryKey' | ||
import { isObject } from '../utils/isObject' | ||
import { Relation, RelationsList } from '../relations/Relation' | ||
import { NullableProperty } from '../nullable' | ||
|
||
const log = debug('parseModelDefinition') | ||
|
||
|
@@ -69,6 +70,12 @@ function deepParseModelDefinition<Dictionary extends ModelDictionary>( | |
continue | ||
} | ||
|
||
if (value instanceof NullableProperty) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't it have the same effect if we omit this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The NullableProperty class gets caught by the if (isObject<NestedModelDefinition>(value) && !(value instanceof NullableProperty)) {
...
} The above is definitely clearer as to why it needs to be there but I kinda like that There may be alternatives around changing the |
||
// Add nullable properties to the same list as regular properties | ||
result.properties.push(propertyPath) | ||
continue | ||
} | ||
|
||
// Relations. | ||
if (value instanceof Relation) { | ||
// Store the relations in a separate object. | ||
|
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 are we checking for nullable properties first? Would some of them fall under
isModelValueType
check below?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 don't think it would be caught by the
isModelValueType
check, I put it there because it felt happy next to theinstanceof PrimaryKey
check haha. I can move it belowisModelValueType
though if you'd prefer?