Skip to content

Session improvements - #98

Open
LarsVomMars wants to merge 9 commits into
zhmushan:masterfrom
LarsVomMars:master
Open

Session improvements#98
LarsVomMars wants to merge 9 commits into
zhmushan:masterfrom
LarsVomMars:master

Conversation

@LarsVomMars

@LarsVomMars LarsVomMars commented Aug 3, 2020

Copy link
Copy Markdown
Contributor
  • Skipper
  • Tests
  • Better typings
  • Session field

Better typings

I already considered this but IMO it's very useful to be able to store data of any kind/type in a session variable without being limited to some basic types.
You probably could serialize the data into JSON on the store side which would get rid of 3 any typings.

Session field

Well I first thought of using a CustomContext for that but this would only complicate things because the user would have to instantiate the CC on every request he wants to access the session data.

~ Lars

@LarsVomMars

LarsVomMars commented Aug 5, 2020

Copy link
Copy Markdown
Contributor Author

Thanks @marvinborner for the tests :)

@LarsVomMars
LarsVomMars marked this pull request as ready for review August 5, 2020 22:54
@LarsVomMars

Copy link
Copy Markdown
Contributor Author

I think we should discuss the last two points on the list because so far I see this, there is not really a better solution to them.

Comment thread middleware/session.ts Outdated
Comment thread docs/session.md Outdated
Comment thread middleware/session.ts Outdated
@zhmushan

zhmushan commented Aug 6, 2020

Copy link
Copy Markdown
Owner

Below is my design:

// session.ts
export const key = Symbol("abc.session")
interface SessionConfig {
  skipper: ...
  // required
  store: ...
}

// this func create a session mw
export function mw(cfg) {}

export function get(name: string, c: Context): Session {
  // const s = c.get(key)
  // return s.get(name)
}

// main.ts
import * as session from "./session.ts"

app.use(session.mw({...}))

app.get("/", c => {
  sess = session.get("session", c)
  // Then, we need a way to set the session options, such as maxAge, etc.
})

@LarsVomMars

LarsVomMars commented Aug 6, 2020

Copy link
Copy Markdown
Contributor Author

Although I don't see how this would improve the workflow yet is justifiable for the user in any way.
Like I mean in this case I could use a CustomContext as well which would lead to a better/more usable version in the end.

--
Didn't see the lastest commit, will look into it later.

@zhmushan

Copy link
Copy Markdown
Owner

@LarsVomMars Sorry for late, thanks for your contribution.
If you agree, I will make improvements directly on your branch, probably next week

@LarsVomMars

Copy link
Copy Markdown
Contributor Author

You free to edit. I can't do this much atm, because my attention is currently drawn to some other projects.

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