Skip to content
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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

3commascapital
Copy link

@3commascapital 3commascapital commented Sep 9, 2024

  • 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(),
Copy link
Collaborator

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?

Copy link
Author

@3commascapital 3commascapital Sep 10, 2024

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.

Copy link
Collaborator

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Copy link
Author

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

@3commascapital 3commascapital force-pushed the add-default-column-values branch 2 times, most recently from a81d766 to 9e317aa Compare September 12, 2024 16:38
Copy link
Collaborator

@kyscott18 kyscott18 left a 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.

packages/core/src/schema/utils.ts Outdated Show resolved Hide resolved
packages/core/src/schema/utils.ts Outdated Show resolved Hide resolved
@@ -123,7 +123,7 @@ const P = {
many,
enum: _enum,
index,
};
} as P;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed

Copy link
Author

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.

packages/core/src/schema/common.ts Outdated Show resolved Hide resolved
packages/core/src/schema/common.ts Outdated Show resolved Hide resolved
packages/core/src/schema/columns.ts Show resolved Hide resolved
packages/core/src/database/postgres/service.test.ts Outdated Show resolved Hide resolved
packages/core/src/database/postgres/service.ts Outdated Show resolved Hide resolved
packages/core/src/schema/columns.ts Outdated Show resolved Hide resolved
@3commascapital 3commascapital force-pushed the add-default-column-values branch 4 times, most recently from 75923eb to 0df9cf6 Compare September 14, 2024 03:11
@3commascapital 3commascapital changed the title Adds defaultTo method hook Adds default schema column hook Sep 15, 2024
@kyscott18 kyscott18 changed the title Adds default schema column hook Add .default() as a modified on column definitions Sep 16, 2024
@kyscott18 kyscott18 changed the title Add .default() as a modified on column definitions Add .default() as column definition modifier Sep 16, 2024
@3commascapital 3commascapital force-pushed the add-default-column-values branch 3 times, most recently from 505cba1 to 3d3911b Compare September 19, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants