kleydon / prisma-session-store

Express session store for Prisma
MIT License
116 stars 18 forks source link

Ability to redefine Session model fields #29

Closed simontaisne closed 3 years ago

simontaisne commented 3 years ago

Feature Request

The following Session model is expected within the schema.prisma for the Prisma session store to work.

model Session {
  id        String   @id
  sid       String   @unique
  data      String
  expires   DateTime
}

It would be great if we could somehow rename those fields. I'm thinking more specifically about the expires field which currently doesn't follow the Prisma "convention" for the DateTime fields (i.e. suffixed with At).

model Post {
  id         Int         @id @default(autoincrement())
  createdAt  DateTime    @default(now())
  ...
}

By the way, thanks for your work on this awesome implementation.

wSedlacek commented 3 years ago

I don't think providing an option to rename fields would necessarily provide a meaningful improvement. That said I am open to renaming that hardcoded field name, but it would be a breaking change. Before making such a breaking change I would like to consider the reasoning for the change.

In the link you referenced I did notice how they had named their fields in the examples but I didn't notice any style guide which explained the rational behind the naming. Do you have any further references or anything in that reference in particular that goes into why this naming is preferred?

kleydon commented 3 years ago

Another consideration to weigh (though not necessarily a dominant one): the Express Session library itself uses the term expires.

wSedlacek commented 3 years ago

Another consideration to weigh (though not necessarily a dominant one): the Express Session library itself uses the term expires.

Given that the context exposed to developers is in the Prisma schema, I believe that would take precedence. However I believe we need good cause to make any breaking changes.

kleydon commented 3 years ago

Given that the context exposed to developers is in the Prisma schema, I believe that would take precedence. However I believe we need good cause to make any breaking change

Makes sense to me.

simontaisne commented 3 years ago

Do you have any further references or anything in that reference in particular that goes into why this naming is preferred?

I haven't found any concrete answer on Prisma explaining the rationale behind the current naming conventions. But the fact is that the entire documentation references the DateTime fields with the xxxAt convention, which is very likely to become the preferred way for Prisma users.

It is also interesting to note that the @updatedAt attribute for automatically setting the time of the last update of a record also follows the same convention.

Before making such a breaking change I would like to consider the reasoning for the change.

As the prisma-session-store implementation targets Prisma users, it seems natural IMO to want to stick to their standards. However I understand that introducing a BC is maybe not the best idea, hence the idea of providing an option to rename fields.

wSedlacek commented 3 years ago

🤔 So I am thinking that maybe it would be better if we exposed a generator that just handled the session table for users rather than expecting them to use a certain format in their schema. This would give us more control over the shape of the session model and perhaps empower more advanced features in the future.

For the public facing API it might look something like this.

generator session {
  provider = "prisma-session-store"
}

The real question is what would be the implantation of such a generator. The idea would really just to take on the responsibility of this relationship internally so that developers don't need to worry about the shape of the session model anymore.

@kleydon do you have any ideas on how we might internalize this relationship?

kleydon commented 3 years ago

@wSedlacek When you say "generator", are you using this a) in the specific sense of generators as introduced within the Prisma schema itself, or b) a broader generator coding pattern? If (a): I don't have much insight into how / whether a 3rd-party library (such as this) can interface with declarations made in Prisma schema files... Unfortunately I don't currently have much time to investigate this, but am open to considering what (a) -- or (b) for that matter -- might look like.

I imagine that Prisma's convention of xxxAt for dates will remain (tenuously based on watching Prisma's development from the outside for the past year and a half.)

I too am questioning the value of introducing the ability to name session model fields arbitrarily, and would want to understand the usage scenarios that might make this beneficial... (@simontaisne - are you solely looking for consistency in this one date-naming convention - or are you looking more broadly for the ability to choose names for session db/model fields?)

At the moment, I'm leaning towards either the "simple" solution (a breaking change that renames "expires" -> xxxAt (expiresAt?) to improve consistency across the wider Prisma ecosystem (particularly while this ecosystem is still young), or else leaving things as they are - while making a mental note to track whether the xxxAt convention remains/deepens...

@wSedlacek - What do you think? Does exploring the generator approach hold any particular appeal for you? Given that (I think?) this approach would also necessitate a breaking change: Would you currently be more inclined to make a simpler (less flexible) breaking change, a more sophisticated (but potentially time-consuming) breaking change, or to leave things as they are for now (perhaps lumping in one of these two changes, when other breaking changes seem necessary)?

wSedlacek commented 3 years ago

I've worked with the Prisma SDK for building generators before. In essense it just passes the Prisma SDL to a function you define and you can use it to generate code or do whatever you like.

In our case we would probably just call the Prisma client generator with some default values to generate a client specifically for sessions.

The one major concern I would have is how it would interface with Prisma Migrate. As far as I am aware Prisma Migrate does not take into account the generators at all.

I do think there is more value in internalizing the model than providing some sort of mapping interface. For one the interface of sessions could be more reliable if it was managed internally. That could allow for optimizations or features to use new fields without the developer needing to update their schema file.

All that being said, I don't have too much free time to work on this either so being pragmatic about it I believe it is best to just leave it the way it is until more breaking changes are required and we can revisit the topic then.

simontaisne commented 3 years ago

I too am questioning the value of introducing the ability to name session model fields arbitrarily, and would want to understand the usage scenarios that might make this beneficial...

I'm personally looking for consistency. But since I believe this convention comes from an opinionated preference from Prisma, I thought it might be a good idea to leave the choice to the end user.

However if it's too hard to accomplish (I don't know much about generators but the limitation with Prisma Migrate should be a blocker IMO), I would personally go for renaming the field expires to improve consistency across the entire Prisma ecosystem.

kleydon commented 3 years ago

To match Prisma's date naming convention, the expires field has been changed to expiresAt, for versions of prisma-session-store 3.0.0 onward.

simontaisne commented 3 years ago

Awesome thanks @kleydon!

kleydon commented 3 years ago

👍