-
Notifications
You must be signed in to change notification settings - Fork 19
Add basic hooks support (v4) #115
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
Conversation
/** | ||
* Represents a type which can release resources, such as event listening or a timer. | ||
*/ | ||
export declare class Disposable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disposable
is not intended to be unique to hooks. You could give it its own file, but since it's a simple class I'd probably just move it directly into index.d.ts
* @param handler the handler for the event | ||
* @returns a `Disposable` object that can be used to unregister the hook | ||
*/ | ||
export function onStart(handler: AppStartHandler): Disposable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards this organization:
app.hook.start
app.hook.terminate
app.hook.preInvocation
app.hook.postInvocation
I'm not a big of "on". I feel like we could've easily done that for the trigger registrations (app.onHttp
, app.onTimer
), and I don't want to be inconsistent and start using "on" for hooks. That being said, I still like grouping all the hooks together to make the Intellisense cleaner. Rather than group them with the "on" prefix, I think a more explicit "hook" group would make sense.
It's a bit weird to have hook
on app
when input
/output
/trigger
aren't on app
, but I feel like it's okay because the latter don't actually register anything.
* @param propertyName The name of the property to set | ||
* @param value The value to set | ||
*/ | ||
setGlobal(propertyName: string, value: unknown): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't want us to feel obligated to follow the core api, especially since we know we want to clean up the hook data. Taking a step back, how is setGlobal
and getGlobal
better than just using a global variable?
Aka rather than this:
const onStartFunc: AppStartHandler = async (context: AppStartContext) => {
context.setGlobal('globalKey', 'test123');
}
app.onStart(onStartFunc);
const onTerminateHook: AppTerminateHandler = async (context: AppTerminateContext) => {
assert(context.getGlobal('globalKey') === 'test123');
};
app.onTerminate(onTerminateHook);
users could do this:
let globalValue: string;
const onStartFunc: AppStartHandler = async (context: AppStartContext) => {
globalValue = 'test123';
}
app.onStart(onStartFunc);
const onTerminateHook: AppTerminateHandler = async (context: AppTerminateContext) => {
assert(globalValue === 'test123');
};
app.onTerminate(onTerminateHook);
If the user is defining their own global variables, they can more easily set the type (aka string
instead of unknown
), and they don't have to deal with the key (mispelling, conflicting with another key, etc.).
Lastly, I think it would simplify our lives if we only need to worry about invocation-scoped hook data, which IMO is something users can't accomplish on their own so it's much more valuable.
* Object passed to PreInvocationContext constructors. | ||
* For testing purposes only | ||
*/ | ||
export interface PreInvocationContextInit extends InvocationHookContextInit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like you told me this already, but I'm confused why we have both PreInvocationContextInit
and PreInvocationCoreContext
, with "core" nested into "init". Can we have just one object without duplication/nesting?
Closing due to lack of activity |
This PR lays the groundwork to add basic support for hooks in v4. This is mostly a thin wrapper over the existing hooks Core API.
Hooks supported
Registering hooks
You can register the hook you desire as below:
Sharing data between hooks
The
hookData
andappHookData
from the core API are replaced withcontext.get
,context.set
(replacinghookData
), andcontext.getGlobal
andcontext.setGlobal
(replacingappHookData
):