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

Support promises #722

Merged
merged 20 commits into from
Jul 25, 2024
Merged

Support promises #722

merged 20 commits into from
Jul 25, 2024

Conversation

josephjclark
Copy link
Collaborator

@josephjclark josephjclark commented Jun 25, 2024

This PR enables all operations in job code to be treated like promises.

This confers the following benefits:

  • fn.catch() is a really useful thing to be able to do
  • We can drop most callbacks from adaptor APIs

Closes #721

Documentation in #OpenFn/docs#518 (this docs PR should be a great non-technical explanation of what this PR actually does)

Motivations and examples

It occurred to me a little while ago that adaptor API design would be way cleaner if we didn't need a callback option as the last argument. It gets very hard to manage optional arguments AND an optional callback.

I also think that the callback function is semantically a little difficult. Why is is there? In most cases, you can rewrite get(www, {}, callback) as get(www, {}); callback(). It's exactly the same thing.

About the only time you need the callback is if you're nested inside an each (or maybe some other operation): each($.data, post('/upsert', $.data, (s) => s)). The final callback here needs to be invoked for each item in the array - an fn() block won't do it.

But if you could treat each operation as a promise, make it thenable and catchable, you wouldn't need the callback.

You can do stuff like this:

each($.data, post('/upsert', (state) => state.data).then((s) => s))

You can also use a catch on any operation now, if for any reason you want to do your own error handling (#496):

get('www').catch(e => { ... })

Compiler magic

It turns out that making operations into promises isn't easy.

I don't want to re-write every adaptor to return a promise. Even if I did, that wouldn't work with the runtime (which expects to execute an array of functions, not an array of promises). Plus the promise would execute immediately, whereas we want to defer execution until later.

I also don't want magic in the runtime to handle this. I think it's important that compiled code can be executed in a simple node.js app without our runtime. That keeps code portable. If the runtime is doing magic to promisify all operations, then the expression isn't very portable.

So the solution goes in the compiler. We have to transform the users's code into something portable.

The tl;dr is, we take a promise fn().then(x) and compile it into tthis:

_defer(fn(), p => p.then(x))`

Where defer is a function imported from the runtime itself, so really the compiled code is like

import { defer as _defer } from '@openfn/runtime';
export default [_defer(fn(), p => p.then(x))]

It's kinda hard to explain, I struggle with the language and this reflects in the implementation. But I'll have a go.

Some important things to understand here:

  • fn() returns a function (maybe an async one) but not a promise
  • The fn() is compiled into an array of functions, which will be passed to the runtime
  • The runtime expects an array of functions to execute, not an array of promises
  • Even if I wanted to change every adaptor to return a promise, that wouldn't really work with the runtime
  • It is critical that the operation created by fn() is not executed immediately. This is the whole thing. Each operation must create a function to be executed later.
  • And obviously when you do promise.then(), the promise will execute right away

My solution is to break up the chain into two parts

  1. The original operation fn(), untouched.
  2. The promise bit, then(whatever). Which might actually be a chain of promises.

I do this by creating a function called defer(operation, thenFn, catchFn)

  • defer is just am operation really. It returns a function that takes state and returns state.
  • When defer is invoked, it'll call the operation fn() and pass the result into Promise.resolve
  • This gives us a Promise that takes and returns state
  • Then we have to trigger the promise bit

The compiler code to break up the promise into a defer function is a little gnarly. I've tested it as well as I can.

Catch also complicates things.

We compile catch() and then() a bit differently. The catch is designed to trap any exception thrown by the operation - even though the operation may not be a promise. So the defer function needs to handle this right.

If for some reason the user does fn().catch().then(), then we have to handle the catch function AND invoke the promise with state.

New Integration Tests

I've added a new suite of integration tests specifically for job writing.

These just compile and run against an adaptor - no CLI or Worker in the way. So it should be a nice clean environment to write, test, debug and experiment with pure job code.

TODO

At the time of writing I've still got a couple of things to look at:

  • catch needs to pass state to the callback
  • The defer function is actually quite long (it pretty prints at like 12 lines) and it distracts quite heavily from the job code. I don't want to minify the function because I want compiled code to be readable. So either I need a prettier/simpler function, or we need to import the defer function from a library (I don't think it's totally unreasonable to import from @openfn/runtime in job code)
  • Compiler stuff is scary and I want to test this so damn hard
  • Docs

@josephjclark
Copy link
Collaborator Author

Taylor 👍 to .then() and .catch() on everything

Taylor 👍 to fn() being the primary canonical means of doing a "callback", but the promise is an option

@josephjclark
Copy link
Collaborator Author

One blocker on removing callbacks:

each($.data.ids, get('www, (s) => s))

then() will save us on v2, but v1 will not have this support

@josephjclark

This comment was marked as resolved.

@josephjclark josephjclark marked this pull request as ready for review July 17, 2024 16:27
@josephjclark
Copy link
Collaborator Author

Hey I think we're ready to go here!

I'd really like Stu to take a look at this next week, because the compiler changes are a bit aggressive.

I'd also love @mtuchi and maybe @taylordowns2000 to have a play with openfnx against this branch and see what you think! Feedback very much apprecaited. Check the docs PR for end-user sort of documentation

@mtuchi
Copy link
Contributor

mtuchi commented Jul 19, 2024

State is undefined in this example 👇🏽

bulkQuery('SELECT Name, Email FROM Contaca').catch((error, state) => {
  console.log({ error, state });
  return state;
});

@josephjclark josephjclark force-pushed the promises-all-the-way-down branch from 056f729 to b51f4c1 Compare July 25, 2024 12:39
@josephjclark josephjclark merged commit f52073a into main Jul 25, 2024
7 checks passed
@josephjclark josephjclark deleted the promises-all-the-way-down branch July 25, 2024 13:01
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.

compiler: support .then() and .catch()
2 participants