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.

Sign up for GitHub

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

Jump to bottom

initialize context.state sooner #1646

Open
dwhieb opened this issue Feb 16, 2022 · 9 comments · May be fixed by #1732
Open

initialize context.state sooner #1646

dwhieb opened this issue Feb 16, 2022 · 9 comments · May be fixed by #1732
Labels
help wanted

Comments

@dwhieb
Copy link

dwhieb commented Feb 16, 2022 •
edited

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:

koa/lib/application.js

Lines 168 to 181 in aa816ca

createContext (req, res) {
const context = Object.create(this.context)
const request = context.request = Object.create(this.request)
const response = context.response = Object.create(this.response)
context.app = request.app = response.app = this
context.req = request.req = response.req = req
context.res = request.res = response.res = res
request.ctx = response.ctx = context
request.response = response
response.request = request
context.originalUrl = request.originalUrl = req.url
context.state = {}
return context
}

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:

koa/lib/application.js

Line 179 in aa816ca

context.state = {}

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 dwhieb changed the title initialize state sooner initialize context.state sooner Feb 16, 2022
@3imed-jaberi
Copy link
Member

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

@miwnwski
Copy link
Member

miwnwski commented Jul 1, 2022 •
edited

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
Copy link
Member

miwnwski commented Jul 2, 2022

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.

@krisstern
Copy link

Hi, I would like to work on this issue.

@krisstern
Copy link

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

@krisstern krisstern linked a pull request Dec 24, 2022 that will close this issue
Open
6 tasks
@iamgabrielsoft
Copy link

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

@dwhieb
Copy link
Author

dwhieb commented Jan 25, 2023

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
Copy link

@dwhieb The change does not work

@siakc
Copy link

siakc commented Dec 25, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Initialize context.state sooner krisstern/koa
6 participants
@dwhieb @siakc @3imed-jaberi @miwnwski @iamgabrielsoft @krisstern