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 method operations #738

Merged
merged 6 commits into from
Jul 29, 2024
Merged

Conversation

josephjclark
Copy link
Collaborator

@josephjclark josephjclark commented Jul 25, 2024

A small fix in the compiler to recognise http.get() as an operation, like get()

This is needed to support the new http namespace in common, see OpenFn/adaptors#692

Discussion

Is this reasonable? Is this going to break everything?

It's fine.

To date, we have assumed that any top level function call, eg, fn(), is an operation call. And as a result it gets relocated into the exports array.

export default [fn()]`

Up to today, any method calls will be left alone. Which may or may not work depending on what you're trying to do.

http.get(www)

Where get is the old non-operation common.http.get.

That probably won't work though, because the get wil be issued before the runtime calls the operation pipeline. It'll all be out of order.

Now, because I'm recognising method calls as operations, I'll do this:

export default [http.get(www)]

If http.get is an operation, this will work. If it's not, well, it shouldn't be at the top of your code, should it? It shoudl be in an fn block. We do say that top level statements should be Operations (it's true, see the last line of the Job Writing Guide )

Copy link
Member

@stuartc stuartc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a few console.logs that need to be taken out, assuming you know they are there.

The change looks good to me. But it does add a small barrier (or consideration) to taking member expressions to the next level (like non operation calls), but we're not there yet. We'd eventually have to know more about where these functions come from.

That's the only thing I thought was worth bringing up. Anyhoo, nice one!

@josephjclark
Copy link
Collaborator Author

@stuartc yeah so this would be a non-operation method call which you can do today:

dateFns.format(blah)

But that has to be nested inside an operation callback anyway. You can't just run that at the top of the script. Or at least you shouldn't.

Interestingly, this should be totally legal at the top of the job (it's actually a pattern I'd like to encourage)

const today = dateFns.format(new Date())

Let me add a test to check it

@josephjclark
Copy link
Collaborator Author

@stuartc And thank you for pointing out the logs! I'd missed them

@josephjclark josephjclark merged commit 5c72cb6 into main Jul 29, 2024
6 checks passed
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 this pull request may close these issues.

2 participants