Conversation
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 |
Collaborator
Author
|
One blocker on removing callbacks:
|
This comment was marked as resolved.
This comment was marked as resolved.
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 |
Contributor
|
State is undefined in this example 👇🏽 |
but still a problem with catch
056f729 to
b51f4c1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 doCloses #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)asget(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 - anfn()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:
You can also use a catch on any operation now, if for any reason you want to do your own error handling (#496):
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:Where defer is a function imported from the runtime itself, so really the compiled code is like
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()is compiled into an array of functions, which will be passed to the runtimefn()is not executed immediately. This is the whole thing. Each operation must create a function to be executed later.My solution is to break up the chain into two parts
fn(), untouched.then(whatever). Which might actually be a chain of promises.I do this by creating a function called
defer(operation, thenFn, catchFn)deferis just am operation really. It returns a function that takes state and returns state.deferis invoked, it'll call the operationfn()and pass the result intoPromise.resolveThe 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()andthen()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: