-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Context State and Hookless hooking #139
Comments
Here is an example using this approach to implement a logger // app usage
import {log} from 'my-logger';
@bind('log.immediately')
const immediate = false;
class MyApplication {
public handle(@inject('writer') writer, @inject('invoke') invoke, @inject('log') log) {
const result = await invoke();
await writer(result);
if (shouldLog()) log();
}
public components = [log];
}
// extension
export const log = [writeLog, getTime, logFormat, getStatus, immediate];
@bind('log')
export function writeLog(@inject('log.format') fmt) {
console.log(fmt);
}
@bind('res.time')
async function getTime(@inject('state.accepted') accepted, @inject('state.finished') finished, @inject('log.immediate') immediate) {
if (immediate) return;
await accepted();
const start = hrtime();
await finished();
return hrtime() - start;
}
@bind('log.format')
function logFormat(@inject('res.time') time, @inject('req.url')) {
return `${url} - ${status} - ${time}`;
}
@bind('log.immediately')
const immediate = true;
@bind('res.status')
async function getStatus(@inject('state.finished') finished, @inject('result.status') status) {
await finished();
return status;
} |
I had to re-read the proposal several times to understand it. I think it may work, although I am concerned whether an average LoopBack user will be able to understand this mechanism. Let's consider an example where the user makes an error and accidentally calls the class MyApplication {
public handle(@inject('writer') writer, @inject('invoke') invoke, @inject('log') log) {
const result = await invoke();
await log();
await writer(result);
}
public components = [log];
} IIUC, this will create a deadlock: |
My understanding of @ritch's proposal is to use
@bajtos The deadlock is caused by the fact that In general, I like the idea to have each component express some requirements so that LoopBack can use them as partial order to sort or trigger such actions. But we need to keep the metadata as simple as possible. IMO, |
I like that idea too. I think your sentence is nicely describing the high-level goal 🎯 |
@ritch I really like your idea, I think is just clean and extensible... That way we wouldn't entirely depend on you supporting your own provided hooks/states, so if I'm correctly understanding we would be able to also create our own states and bind these custom states, inject these in our controller methods and execute the flow as we desire? If that is the case, that would be pretty cool, I did read it a couple of times (or more) also to understand and still not entirely sure I understood well, but this looks promising. So yes it might be a little complicated at the beginning, but with good documentation and a good example I think it might be straight forward. My two cents. |
@ritch If I remove all the syntax sugars, your proposal becomes much simpler to understand:
{
'log': writeLog, // function
'res.time': getTime, // function
'log.format': logFormat, //function
'res.status': getStatus, // function
'log.immediately': immediate // variable?
}
{
'log': ['log.format']
'res.time': ['state.accepted', 'state.finished', 'log.immediately']
'log.format': ['res.time', 'req.url']
'log.immediately': []
'res.status': ['state.finished', 'result.status']
} There are a few issues in your original example:
|
Furthermore, relations between participants of the request processing can be expressed in one of the following forms:
|
I think this proposal has been superseded by https://github.com/strongloop/loopback-next/wiki/Sequence. @ritch feel free to reopen if you disagree. |
@bajtos I think we should keep this issue open. IMO,
These strategies can be bound to the app ctx to replace the default imperative implementation. |
@raymondfeng sounds good, thank you for reopening the issue then. |
See #598 |
This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the |
This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the |
@simonho @Raymond @miroslav
I have a proposal for hooks that re-uses our existing declare context bindings approach.
The way this works is that you can ensure some state has already occurred, but not define a specific order of execution. Which gives you the benefits of koa without the middleware execution order dependency.
The text was updated successfully, but these errors were encountered: