-
Notifications
You must be signed in to change notification settings - Fork 14
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
[name bikeshed] This API is useful in both async and sync situations #24
Comments
We'll take this under advisement. I think @michaelficarra also suggested a few names?
I wouldn't advise this. While it's usable in sync contexts, the API is going to have some overhead during run/get operations. We're going to try to make this fast, but if you could use another solution, it's likely that whatever that is will be faster overall. |
In many contexts, cleanliness trumps absolute perf though, and this API with a name that doesn't include "async" could convey the intention better than the There are other examples already in the language, like |
|
That's a good point. Indeed, in Safari, once the best JIT kicks in |
There's a subtle difference when using this API in a synchronous way compared to let normalVar = undefined;
const asyncVar = new AsyncContext.Variable();
// prints 123
asyncVar.run(123, () => {
setTimeout(() => console.log(asyncVar.get()), 1000);
});
// prints undefined
const oldNormalVar = normalVar;
normalVar = 123;
try {
setTimeout(() => console.log(normalVar), 1000);
} finally {
normalVar = oldNormalVar;
} The name being I find the second (synchronous-only) solution common enough that const syncVar = new SyncContext.Variable();
// prints undefined (or throws an error)
syncVar.run(123, () => {
setTimeout(() => console.log(syncVar.get()), 1000);
}); |
One name already used in the ecosystem is "continuation local storage" which is nice since it isn't specific to sync or async. See for example https://www.npmjs.com/package/continuation-local-storage with ~2m weekly downloads. That's also used by some popular libraries directly (e.g. Sequelize) so there's some familiarity with it. |
I would actually suggest against reusing an existing name as it may result in confusion between the two. If we really want something agnostic of sync versus async, perhaps something like |
This name AsyncContext has actually become a bit confusing as I've started to pitch reusing this mechanism to the web framework community, for other things that are commonly called "context" and, similarly, boil down to saving and restoring variable maps along the callstack for later execution. But I'm not sure what a better name is... at this point, the AsyncContext name has been shared in a lot of contexts and seems to be "sticking". |
This is IMO a much needed addition to the language, I love this proposal.
I wonder however if calling it
AsyncContext
is the best way to go about it, given that it is also useful in sync contexts, and could replace thetry{}finally{}
pattern described in the README in every situation.While we're at it, given that
this
is already the context in JS, another term may be less confusing. I know that React also usescontext
for its dynamic scope construct, so there's also a precedent here... No strong opinions, just raising this in case we find something better.The text was updated successfully, but these errors were encountered: