-
Notifications
You must be signed in to change notification settings - Fork 89
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 .default()
as column definition modifier
#1085
base: main
Are you sure you want to change the base?
Add .default()
as column definition modifier
#1085
Conversation
3commascapital
commented
Sep 9, 2024
•
edited
Loading
edited
- add hook for use in schema
- use default for method name
- write tests to verify that default value is used
id: p.string().defaultTo("0"), | ||
name: p.string().defaultTo("firstname"), | ||
age: p.int().defaultTo(5).optional(), | ||
bigAge: p.bigint().defaultTo(5n).optional(), |
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.
Couple general notes:
- Per the discussion here I propose we go with
.default()
over.defaultTo()
- I'm not sure if this is handled yet, but adding
.default
to a column should automatically/implicitly make it optional. Perhaps we should validate against including both.optional()
and.default()
to a column?
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.
ah, thanks for linking. forgot to do that. regarding your points:
- if we do not mind the js
.default
export pattern and wish to distinguish from this pattern, i am ok with that. That possible conflict was the only one I had in mind. - I don't know that it should actually. Optional only denotes nullability, which should still be possible to do even if you started with a non null value. In other words, when I first insert, I don't have a value, the database will use the default value for me, but I should be able to turn that into null later on or use null during insertion.
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.
Re 1: I think it's fine - this is being used in a context rather distant from a module export. Fwiw Drizzle uses .default
as well.
Re 2: Ah yes, my mistake - you're right. The interesting case is: if a column has a default and is NOT optional, it should still be optional when inserting or updating records of that table (but not when selecting). I'm not sure how well our existing type system is set up to handle this, cc @kyscott18
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 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 (probably) be addressable in this pr
a81d766
to
9e317aa
Compare
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.
Nice work - reviewed most of this. I think we need to add extra logic to database/sqlite/service.ts
and schema/infer.ts
but looking good.
@@ -123,7 +123,7 @@ const P = { | |||
many, | |||
enum: _enum, | |||
index, | |||
}; | |||
} as P; |
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 is this needed
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.
without this coersion, i get the following error during typecheck:
packages/core build: src/schema/schema.ts(112,7): error TS4023: Exported variable 'P' has or is using name 'DefaultTo' from external module "~/ponder/packages/core/src/schema/columns" but cannot be named.
75923eb
to
0df9cf6
Compare
.default()
as a modified on column definitions
.default()
as a modified on column definitions.default()
as column definition modifier
505cba1
to
3d3911b
Compare
Adds json insertion
3d3911b
to
8847088
Compare