koajs / koa

Expressive middleware for node.js using ES2017 async functions
https://koajs.com
MIT License
35.21k stars 3.23k forks source link

initialize `context.state` sooner #1646

Open dwhieb opened 2 years ago

dwhieb commented 2 years ago

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.

3imed-jaberi commented 2 years ago

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

krisstern commented 1 year ago

Hi, I would like to work on this issue.

krisstern commented 1 year ago

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

iamgabrielsoft commented 1 year ago

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

dwhieb commented 1 year ago

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.

krisstern commented 1 year ago

@dwhieb The change does not work

siakc commented 10 months ago

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.

jonathanong commented 2 months ago

what's the use-case for this? I understand what you want, but I don't know why you want it.

jonathanong commented 2 months ago

it's more likely going to cause issues because someone is going to not understand that a subset of the state is shared across requests

dwhieb commented 2 months ago

@jonathanong This is useful for any piece of data that the user would like available on all/most requests. This is standard behavior in other frameworks as well. See especially Express:

https://expressjs.com/en/5x/api.html#app.locals

jonathanong commented 2 months ago

you're saying what you want, not why you want it. just because others do it is not a good reason for us to do it too

the express examples are definitely bad - you don't want to add a module as a local. titles belong in your templating system (which Koa does not have, unlike Express). I have no idea why you'd have an email address as an app-level local

dwhieb commented 2 months ago

In general, having app-level context is useful whenever you need to read data from a file or database, but don't want to retrieve that data for every request. You can read the file/database once when the app initializes, rather than on every request.

Variables I've made available to all requests in the past include:

jonathanong commented 2 months ago

can you show some code example please? why would you attach it to the koa framework instead of using a singleton module?

dwhieb commented 2 months ago

No, the projects I was using Koa for have transitioned to Express. But here's a similar approach with Express:

https://github.com/dwhieb/Nisinoon/blob/main/app/locals.js

I'd attach it to the Koa framework's context object because the context object is the logical space that's been set aside for exactly those types of variables. Otherwise users have to remember which variables are stored in context and which are stored in the singleton.

Per Koa's documentation:

ctx.state

The recommended namespace for passing information through middleware and to your frontend views.

This strongly suggests that best practice is to store context variables here rather than in a separate singleton.

kevinpeno commented 3 weeks ago

I'm still unsure why this should be in core, for a few reasons.

First, Koa has a documented philosophy that context (including state) is unique to each request.

A Context is created per request, and is referenced in middleware as the receiver, or the ctx identifier

Second, your initial suggestion to "update context" seems to be in line with the documentation as well, as it states the following and does not require a Koa update (emphasis mine):

app.context is the prototype from which ctx is created. You may add additional properties to ctx by editing app.context. This is useful for adding properties or methods to ctx to be used across your entire app, which may be more performant (no middleware) and/or easier (fewer require()s) at the expense of relying more on ctx, which could be considered an anti-pattern.

Also, note that that statement explicitly says you are opting into an anti-pattern by going about things the way described in this issue and in the associated PR.

Finally, context.state documents well how it should be interacted with through middleware to ensure state is how you want it in your frontend views.

ctx.state The recommended namespace for passing information through middleware and to your frontend views.

So it seems to me that the best thing to do would be to either work with the establized patterns and create a framework that supports your application's per-request need for a globalized state via middleware or use the anti-pattern to throw the object directly on context.

I think @siakc's example perfectly expresses how to address your needs.

app.use((ctx, next)=> {ctx.state.sth = 'something'; await next();}

Where your middleware gets this global state from "somewhere" and merges it with the context state during request processing.