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

compiler: support .then() and .catch() #721

Closed
josephjclark opened this issue Jun 25, 2024 · 3 comments · Fixed by #722
Closed

compiler: support .then() and .catch() #721

josephjclark opened this issue Jun 25, 2024 · 3 comments · Fixed by #722
Assignees

Comments

@josephjclark
Copy link
Collaborator

Tricky one this.

Every operation is a promise*. So we should be able to do this in our job code:

fn(s => s).then(s)

This pattern eliminates the need for callbacks. Which is great, because operations with an optional options object and optional callback (which happens a lot) have really ugly signatures.

It also enables this:

fn(s => s).catch(s)

Which is badass! Suddenly I can catch any error I want! Which is covered by OpenFn/adaptors#496 (an issue we have with this is that really I want to pass state and error into the catch, like catch( (e, s) => {}) - but I'm not quite sure how to do that technically. Maybe a custom promise?)

The PROBLEM is that the compiler hates this.

The compiler wants to move all top-level operations into an array, like this:

export default [fn(s => s).then(s)]

First off I think this breaks everything in the runtime, because that function must be deferred. The .then() will immediately invoke it. So I think the compiler needs to do this?

export default [() => fn(s => s).then(s)]

I don't know, I need to wrap my brain around that.

But secondly, and what this issue is about, is the compiler doesn't recognise x.then() as an operation. It recognises it as a method call and ignores it.

I think basically the compiler needs to treat x().then() and x().catch() specially and do some kind of magic wrapping around them to make them work properly.

  • Every job compiles to an array of functions, where each function returns an Operation (a function which takes state and returns state and may itself be a promise). Each of those operations is actually wrapped by our own Promise. So even if an operation is fn(), which is not a promise, that will be wrapped in a promise at execution time. Hurrah!
@josephjclark josephjclark changed the title compiler: allow operations to be treated like promises compiler: support .then() and .catch() Jun 25, 2024
@josephjclark
Copy link
Collaborator Author

If I can get this working, we can open up a debate about removing callbacks from all operations except iterators. Which I think would be a HUGE win

@josephjclark josephjclark mentioned this issue Jun 25, 2024
4 tasks
@josephjclark josephjclark reopened this Jun 26, 2024
@josephjclark josephjclark self-assigned this Jul 12, 2024
@josephjclark
Copy link
Collaborator Author

josephjclark commented Jul 12, 2024

This has taken some working out, and I'm still sure I'm there

fn(args).then(whatever)

Must compile to:

const f = fn(args)

export default [
  (s) => promisify(f(s)).then(whatever)
]

Which boils down to:

export default [ (s) => promisify(fn(args)(s)).then(whatever) ]

Where promisify will take the Operation returned by the operation factory (the function returned by fn(), and if the result of that operation is not a promise, it will be converted into a promise.

It's hairy and complex.

But this has to be compiler magic. I don't want special runtime requirements.

@josephjclark
Copy link
Collaborator Author

Ah, Promise.resolve would mean I don't need to create that silly helper:

So the long form is:

export default [
	(s) => {
		const operation = fn(args)
		const result = operation(s);
		return Promise.resolve(result).then(whatever)
	}
]

Which boils down to:

export default [
	(s) => Promise.resolve(fn(args)(s)).then(whatever)
]

Still a little hairy but it should work.

If the operation returns a value, it'll be passed into Promise.resolve, and straight onto the .then

If the operation returns a promise, it'll wait for that to return before triggering the then.

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 a pull request may close this issue.

1 participant