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

Allow prisma.schema to use Json data type for session data #101

Closed
wants to merge 5 commits into from

Conversation

hugil
Copy link

@hugil hugil commented Jul 30, 2022

Increases flexibility of storing type safe data from auth serialization

Increases flexibility of storing type safe data from auth serialization
@kleydon
Copy link
Owner

kleydon commented Jul 30, 2022

Hi @hugil - thanks for the fix! Looks like a great improvement.

When I try to run the tests, in src/@types/prisma.ts I encounter:

"Cannot find name 'JsonValue'."

Where does this type live? Can you add it via import to "src/@types/prisma.ts"?

Thanks again for the fix.

import { JsonValue } from "type-fest";
@hugil
Copy link
Author

hugil commented Jul 30, 2022

Hi @hugil - thanks for the fix! Looks like a great improvement.

When I try to run the tests, in src/@types/prisma.ts I encounter:

"Cannot find name 'JsonValue'."

Where does this type live? Can you add it via import to "src/@types/prisma.ts"?

Thanks again for the fix.

Hey, thanks for spotting that. Missed the import. I've committed it now so the test should run fine!

kleydon
kleydon previously approved these changes Jul 30, 2022
@kleydon
Copy link
Owner

kleydon commented Jul 30, 2022

Hi @hugil ,
Looks like some of the tests are still failing...
Would you mind running npm run test (and npm run lint) from the project directory, to see what's going on?
(I'll be offline for much of the rest of today, but back again tomorrow.)

@kleydon kleydon dismissed their stale review July 30, 2022 23:40

Tests not yet passing

@gLenczuk
Copy link

gLenczuk commented Aug 4, 2022

It would be good to mention in README, that you can still use string datatype. Now it seems like JSON is only available. And tests are failing because now serializer accepts only a string. You can try to extend this to also accept JSON, but I'm not sure how it would behave.

@kleydon
Copy link
Owner

kleydon commented Aug 21, 2022

@gLenczuk - Good point! It does feel like the docs need to be updated, for it to be clear that string data can still be used...

@hugil - How likely is it that you will finish working on this PR? (If you don't continue, I'll probably try to finish it off - but could be a little while; I'm under water here with other work.

Would love to get (both of) your advice re: whether the changes in this PR constitute a breaking change.

I'm unclear on whether the session's data field has always implicitly been json (such that formally typing it as json shouldn't cause trouble for existing projects, existing sessions in existing dbs, etc), or whether it is possible that the data field could (for some people, in some projects) represent non-json string, and result in problems when upgrading this library. What do you think?

@kleydon
Copy link
Owner

kleydon commented Sep 19, 2022

@hugil, @gLenczuk - on reflection, it looks this prisma-session-store issue needs an alternate approach (if it is still a problem; I'm using prisma 4.3.0, with the current version of prisma-session-store, and not encountering any type issues related to string vs. json...). According to Prisma's database features table, not all dbs support json as a type - which suggests that where the db is concerned, we should probably stick with the lowest common denominator (string) even though @hugil I agree, json would provide greater type safety.
Going to close this for now, and if you have any better ideas - would love to hear them.

@kleydon kleydon closed this Sep 19, 2022
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.

None yet

3 participants