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 support for Drizzle sqlite integer timestamp mode #1629

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

Conversation

LinusOP
Copy link
Member

@LinusOP LinusOP commented Jul 10, 2024

Fixes #1628

Quite a simple thing to support, simply checks if Drizzle already returned a date for us and if so it skips the conversion.
I've tested this in an existing project I had using the Drizzle adapter with SQLite and it worked without issue.

Also added a small comment to explain when this might occur.

Copy link
Member

@pilcrowonpaper pilcrowonpaper left a comment

Choose a reason for hiding this comment

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

Can you also add tests? You can just copy the sqlite.ts test file and change the column types

packages/adapter-drizzle/src/drivers/sqlite.ts Outdated Show resolved Hide resolved
@LinusOP LinusOP requested a review from pilcrowonpaper July 11, 2024 16:12
@LinusOP LinusOP changed the title Add support for sqlite integer timestamp mode Add support for Drizzle sqlite integer timestamp mode Jul 11, 2024
@LinusOP
Copy link
Member Author

LinusOP commented Aug 7, 2024

@pilcrowonpaper Any chance for a review on this?

@pilcrowonpaper
Copy link
Member

Yeah I was wondering if I could @AndriiSherman to confirm that using is() would be safe across different drizzle versions

@pilcrowonpaper
Copy link
Member

Honestly might be smarter and safer to have export DrizzleSQLiteAdapter and DrizzleSQLiteAdapterTimestamp

@LinusOP
Copy link
Member Author

LinusOP commented Aug 8, 2024

Honestly might be smarter and safer to have export DrizzleSQLiteAdapter and DrizzleSQLiteAdapterTimestamp

Yea tbh I don't really mind the specific implementation. Whatever you think is best. Although if we do it like that I reckon they should probably share some logic where applicable. Want me to do a branch of it to see what it'd look like?

@pilcrowonpaper
Copy link
Member

I don't really might code duplication that much but yeah can you do a quick prototype?

@AndriiSherman
Copy link

Yeah I was wondering if I could @AndriiSherman to confirm that using is() would be safe across different drizzle versions

is was designed specifically to check if an object is a Drizzle object and should work across monorepos, different versions of Drizzle, etc. We were using instanceof, but over a year ago, we switched to this helper.

And just to clarify, which different versions of Drizzle are you referring to?

@LinusOP
Copy link
Member Author

LinusOP commented Aug 8, 2024

I don't really might code duplication that much but yeah can you do a quick prototype?

Good to know, don't need to do anything complicated then. Will do a prototype and push it on a different PR when I get home, unless you want it pushed in this branch instead?

@pilcrowonpaper
Copy link
Member

@AndriiSherman Anything above 0.29.0

@pilcrowonpaper
Copy link
Member

@LinusOP You can create a branch and probably just share the link here

@LinusOP
Copy link
Member Author

LinusOP commented Aug 9, 2024

@LinusOP You can create a branch and probably just share the link here

Here you go: https://github.com/LinusOP/lucia/tree/drizzle-sqlite-timestamp-separate
Bear in mind, threw this together super quick so very well might've missed something obvious

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.

[Feature Request]: Support timestamp mode for Drizzle SQLite expiry column
4 participants