-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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.
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
initialize context.state
sooner
#1646
Comments
I like to see |
Hi, I would like to work on this issue. |
I am wondering if there is a development guide for contributors somewhere in the repo? |
this has not being solved yet, i ran into this today |
Reading my initial issue again, I'd actually recommend using |
@dwhieb The change does not work |
|
what's the use-case for this? I understand what you want, but I don't know why you want it. |
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 |
@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: |
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 |
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:
|
can you show some code example please? why would you attach it to the koa framework instead of using a singleton module? |
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 Per Koa's documentation:
This strongly suggests that best practice is to store context variables here rather than in a separate singleton. |
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.
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):
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,
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.
Where your middleware gets this global state from "somewhere" and merges it with the context state during request processing. |
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:Best practice for Koa is to use
context.state
to pass information through middleware, and to useapp.context
add properties used throughout the app, so this seems like the right approach. It's also parallel to the way that I can setapp.locals
in an Express app to make that information available tores.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
The following also doesn't work, because the
state
property gets overridden by the code linked above:Suggested Solution
Add the
state
property to the context prototype instead of initializing it when a request comes in. Then, inapp.createContext()
, create a newstate
object that only lives for the lifetime of the request, usingapp.context.state
as its prototype.Replace:
koa/lib/application.js
Line 179 in aa816ca
with:
Workaround
The workaround is to add the information directly to
app.context
rather thanapp.context.state
. However, this is undesirable because it goes against the best practice of usingcontext.state
to encapsulate state.I'd be happy to open a PR for this if it's a change you'd like implemented.
The text was updated successfully, but these errors were encountered: