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

Remove "default" options from invocation. #124

Closed
avillega opened this issue Jun 13, 2024 · 1 comment
Closed

Remove "default" options from invocation. #124

avillega opened this issue Jun 13, 2024 · 1 comment
Assignees
Labels
enhancement New feature or request

Comments

@avillega
Copy link
Contributor

avillega commented Jun 13, 2024

Describe the problem you are facing

"Default" options, note the quoting, are not truly defaults. Depending on where you are in the execution path they could mean two things:

  1. if in resonate.run: The options used when registering a top level function
  2. if in ctx.run: The options of the parent invocation

This "defaults" are then passed to the Invocation object just to be used in the split function which assigns "defaults" to some options when getting them from the argsOrOpts argument.

I think in neither case we should pass "default" opts to the invocation, we should always be in a position to pass a whole options object to the invocation and only use that one.

For ctx.run I think using the opts of the top level invocation as defaults of child invocations might be an undesirable pattern. for example, I can see how someone might want to set the timeout for a top level invocation very high but still expects a more sensible default timeout for the children invocations. With the current implementation all the children invocations will have the same timeout of the parent invocation.

Describe the solution you'd like

  • Remove the "default" options from the invocation object completely. This would simplify the invocation and its creation.
  • Use a proper default set of options when using ctx.run. This should be the sensible defaults that we use when registering a resonate function.
  • Rename the "default" options in resonate.run to something like registered options.
  • Refactor the split functions (plural intended) This is a major point of complexity and I think most of it could be avoided by keeping the split function a simple pure function that check if the last argument of a variadic set of arguments has the {_resonate: true} tag and returns {args, opts} defaulting to empty options. Each user of this function then handles the set of default options that wants to use.

Additional context

I am currently trying to find the right set of options to store as part of the DurablePromise, DeferredExecution and Top level executions might need to store some of the options in the durable promise, since the set of options that each invocation uses is tangled figuring out what to do, is complicated. I am trying to untangle the options story in the typescript sdk, so the options that we want to store become very clear.

@avillega avillega added the enhancement New feature or request label Jun 13, 2024
@avillega avillega self-assigned this Jun 13, 2024
@avillega
Copy link
Contributor Author

Mostly fixed at #125 with slight differences. Options still inherit from Resonate and the Registered options for the top level call of the current call graph.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant