koa icon indicating copy to clipboard operation
koa copied to clipboard

initialize `context.state` sooner

Open dwhieb opened this issue 3 years ago • 14 comments

Expected Behavior

If there's information I want to be available in every request, I'd like to be able to specify that information when I initialize the app, using app.context.state, like so:

const app = new Koa()
app.context.state.prop = true // TypeError: Cannot set properties of undefined (setting 'prop')
app.use(ctx => console.log(ctx.state.prop))

Best practice for Koa is to use context.state to pass information through middleware, and to use app.context add properties used throughout the app, so this seems like the right approach. It's also parallel to the way that I can set app.locals in an Express app to make that information available to res.locals.

Actual Behavior

The above code snippet throws a TypeError because context.state isn't initialized until the app receives a request, here:

https://github.com/koajs/koa/blob/aa816ca523e0f7f3ca7623163762a2e63a7b0ee3/lib/application.js#L168-L181

The following also doesn't work, because the state property gets overridden by the code linked above:

const app = new Koa()
app.context.state = { prop: true }
app.use(ctx => console.log(ctx.state.prop)) // returns undefined

Suggested Solution

Add the state property to the context prototype instead of initializing it when a request comes in. Then, in app.createContext(), create a new state object that only lives for the lifetime of the request, using app.context.state as its prototype.

Replace:

https://github.com/koajs/koa/blob/aa816ca523e0f7f3ca7623163762a2e63a7b0ee3/lib/application.js#L179

with:

// context.state = Object.assign({}, context.state) // <- old suggestion
context.state = Object.create(context.state)        // <- this is probably better

Workaround

The workaround is to add the information directly to app.context rather than app.context.state. However, this is undesirable because it goes against the best practice of using context.state to encapsulate state.

const app = new Koa()
app.context.prop = true
app.use(ctx => console.log(ctx.prop)) // returns true

I'd be happy to open a PR for this if it's a change you'd like implemented.

dwhieb avatar Feb 16 '22 22:02 dwhieb

I like to see Set or Map here. It should behave better. @miwnwski ?!

3imed-jaberi avatar Mar 25 '22 22:03 3imed-jaberi

If you want some info on every request, you should just modify context as you described. State should be reserved for per request specific data/state, or you can just add a middleware to set it for each request. In other words, there nothing wrong with doing e.g. app.context.db = new Knex(knexConfig)

Changing app.context.state type and/or its behaviour would be a pretty significant semver breakage though.

Edit:

I see, I read this a bit too fast before responding. I guess what would be more appropriate would be to expose the state object initialisation through Application instead of changing its type.

Though I'm not as involved anymore so I have no strong opinions about this.

miwnwski avatar Jul 01 '22 11:07 miwnwski

So after having slept on this I think a PR that makes context creation an overridable Koa option is the way to go. If anyone wants to take a stab at this it would be welcome.

miwnwski avatar Jul 02 '22 09:07 miwnwski

Hi, I would like to work on this issue.

krisstern avatar Nov 30 '22 14:11 krisstern

I am wondering if there is a development guide for contributors somewhere in the repo?

krisstern avatar Dec 24 '22 10:12 krisstern

this has not being solved yet, i ran into this today

iamgabrielsoft avatar Jan 25 '23 01:01 iamgabrielsoft

Reading my initial issue again, I'd actually recommend using Object.create() instead of Object.assign() for the change. I've edited the original comment to reflect this.

dwhieb avatar Jan 25 '23 01:01 dwhieb

@dwhieb The change does not work

krisstern avatar Jan 25 '23 02:01 krisstern

app.use((ctx, next)=> {ctx.state.sth = 'something'; await next();} Put this before all MWs and you are good to go. A context has no meaning without a request. So the context can be valid in MWs chain only.

siakc avatar Dec 25 '23 09:12 siakc