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.

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

Add options to invocation context #54

Merged
merged 3 commits into from
Jan 30, 2023
Merged

Add options to invocation context #54

merged 3 commits into from
Jan 30, 2023

Conversation

ejizba
Copy link
Contributor

@ejizba ejizba commented Jan 26, 2023

Fixes #52

The main goal is just to give people an easy way to inspect the configuration they used to register the function at the time of invocation.

types/InvocationContext.d.ts Show resolved Hide resolved
@@ -53,6 +54,7 @@ export class InvocationModel implements coreTypes.InvocationModel {
retryContext: fromRpcRetryContext(req.retryContext),
traceContext: fromRpcTraceContext(req.traceContext),
triggerMetadata: fromRpcTriggerMetadata(req.triggerMetadata, this.#triggerType),
options: fromRpcBindings(this.#bindings),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My biggest issue with this implementation is that this now requires the options as input to the constuctor of InvocationContext, making the unit testing scenario harder. This code you have in the Wiki here would no longer work:

const testInvocationContext = new InvocationContext({
  functionName: 'testFunctionName',
  invocationId: 'testInvocationId',
  logHandler: (_level, ...args) => console.log(...args)
});

Is there a way around this? Could we have the options object initialized inside the constructor, or set here after the context = new InvocationContext({}) line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this feedback applies to all properties on InvocationContextInit? I've updated so in testing scenarios you can just do new InvocationContext()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great! Yeah, I was gonna suggest that too but in a separate PR 😛

Copy link
Contributor

@hossam-nasr hossam-nasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Can we get a release of the library with these change so I can ingest it and make the necessary change on the SDK side?

@@ -53,6 +54,7 @@ export class InvocationModel implements coreTypes.InvocationModel {
retryContext: fromRpcRetryContext(req.retryContext),
traceContext: fromRpcTraceContext(req.traceContext),
triggerMetadata: fromRpcTriggerMetadata(req.triggerMetadata, this.#triggerType),
options: fromRpcBindings(this.#bindings),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great! Yeah, I was gonna suggest that too but in a separate PR 😛

@ejizba ejizba merged commit 250fd24 into v4.x Jan 30, 2023
@ejizba ejizba deleted the ej/options branch January 30, 2023 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants