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

runtime: crash occurs if an expression doesn't return state #737

Closed
josephjclark opened this issue Jul 19, 2024 · 5 comments · Fixed by #832
Closed

runtime: crash occurs if an expression doesn't return state #737

josephjclark opened this issue Jul 19, 2024 · 5 comments · Fixed by #832
Assignees

Comments

@josephjclark
Copy link
Collaborator

This should suffice:

fn() => undefined)

The runtime blows up with a exception if no state is returned at the end. We need to be more robust.

Also, warn if a operation does not return state

@doc-han
Copy link
Collaborator

doc-han commented Nov 21, 2024

@josephjclark
I'm picking this up. You can do assignment.

@josephjclark
Copy link
Collaborator Author

@doc-han Thanks so much!

You can @ me if you need anything or just raise a rough PR to show direction.

I've added you as a contributor to the repo so you can work straight on-branch if you want (it's a bit easier for me to handle a branch than a fork)

@doc-han
Copy link
Collaborator

doc-han commented Nov 25, 2024

@josephjclark
If an expression doesn't return state, do we want to set state to

  1. empty object? - return {}
  2. inputState with empty data object - return {...inputState, data: {}}
  3. inputState(same as state after previous execution) - return inputState

@josephjclark
Copy link
Collaborator Author

@doc-han let's think about this. Actually there aren't many details in this issue at all

Some important context:

  1. Because of how the runtime execution pipeline works, every Operation is expected to return a state object (which gets passed onto the next operation)
  2. It's super common for users to not realise they have to do this, or to forget. So the next operation blows up because state is nullish
  3. But also there are some use-cases when I don't want to return state. The last operation in the workflow, for example, often doesn't need to return anything.

I think a really good solution here would look like this:

  1. If an OPERATION doesn't return something truthy, make it return some special value [maybe null? -1? A magic class? Null should be fine]
  2. If the NEXT operation starts with the special value:
    2a. console.warn that the previous OPERATION didn't return state, did you forget? (I can help with wording)
    2b. Default the state to {} for the next operation
  3. If a Job in a Workflow starts with the special value:
    3a. console.warn that the previous JOB didn't return state, did you forget? (I can help with wording)
    3b. Default the state of the job to {} (maybe be careful here because assembling state at the start of a job is over-complicated)

Basically I'm trying to:
a) let the workflow run for as long as it possibly can
b) Raise warnings but only if it matters (I don't want to warn for every operation that returns null if it doesn't matter or its intentional)
c) Handle a sublte difference between job state and operation state

@josephjclark
Copy link
Collaborator Author

I suppose there's a danger of over-engineering this 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants