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: allow function calls on lazy state #835

Merged
merged 4 commits into from
Dec 11, 2024

Conversation

doc-han
Copy link
Collaborator

@doc-han doc-han commented Dec 3, 2024

Short Description

Currently function calls on lazy-state do not work.
eg.

fn((state) => {
   state.callMeMaybe = () => {}
  return state
});
// does not work before this PR
fn($.callMeMaybe())

This is because, we fail to wrap the state callback well. This update fixes that.

How it worked before and after

Given the source code fn($.callMeMaybe())

Before this PR After this PR
fn((state => state.callMeMaybe)()) fn(state => state.callMeMaybe())

Fixes #684

Implementation Details

Steps

  1. after finding the MemberExpression with $, we start walking up the tree to find a CallExpression to wrap.
  2. when we find this CallExpression, a simple check is done to find out whether the parent of this CallExpression is another CallExpression with callee being state. (that's how we know it's a function called on a lazy-state)
  3. we then want to wrap the state stuff at an additional level up for it to work!

QA Notes

List any considerations/cases/advice for testing/QA here.

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

@doc-han
Copy link
Collaborator Author

doc-han commented Dec 3, 2024

@josephjclark I want to add a good runtime/cli test for this. where do you think would be best to add that.

fn((state) => {
    state.callMeMaybe = (value) => {
        state.greetings = `hello ${value}`
		return state; // crazy! seems every function body that does something with state has to return it
    }
    return state
});

fn(state => {
    state.name = "John"
    return state;
})

fn($.callMeMaybe($.name)) // note: if callMeMaybe doesn't return state, next operation wouldn't have state passed.

Then the value at state.greetings will be "hello John"

Note

We can say that. every function body that has something to do with state should return it.

@doc-han
Copy link
Collaborator Author

doc-han commented Dec 3, 2024

Note: I ignored the other example from #684

fn((state) => {
   $.callMeMaybe()
})

Because according to our discussion. that should be ILLEGAL!

@doc-han doc-han changed the title feat: allow function calls on lazy state Compiler: allow function calls on lazy state Dec 3, 2024
@josephjclark
Copy link
Collaborator

every function body that has something to do with state should return it.

Uh, well not really. It depends on how it's used. We talk about Operations - a function which takes state and returns state. If you're using a function as an operation - then yeah, the function body must return state.

But I can re-write your code like this:

fn((state) => {
    state.callMeMaybe = (value) => {
        state.greetings = `hello ${value}`
    }
    return state
});

fn(state => {
    state.callMeMaybe(); // no need to return state in this case
    return state;
})

But I think you're hitting on something important. Since we can't use lazy state inside a callback, the use of this is limited.

If I do:

fn($.callMeMaybe($.name))

Then the function must indeed be an Operation.

We can do stuff like this though

fn((state) => {
  state.buildUrl = (resource) => `api/v1/${resource}`
  return state;
});
get($.buildUrl(resourceName))

But thinking this through is making me really de-value the utility of functions on state. I think it's just creating confusion.

#648 would be a much more useful facility (although the issue needs a lot of love!)

@josephjclark
Copy link
Collaborator

@doc-han let me think about this but this does feel low priority now 🤔 If it feels safe I'll merge it

@josephjclark
Copy link
Collaborator

Hiya @doc-han - I want to merge this even though I don't think it's very valuable in the end.

We have a suite of tests in integration-tests/execute which test the compiler and runtime on job code. This is quite new and we don't have many - but it's nice to be able to test job code without the overhead of the CLI (or indeed the multi-process worker engine).

Can I ask you to create a file called lazy-state.test.ts in there and add a few simple lazy state tests and then a test or two on this specifically?

Don't go too overboard - just plug in a nice starting point :)

Thanks!

@doc-han
Copy link
Collaborator Author

doc-han commented Dec 6, 2024

Can I ask you to create a file called lazy-state.test.ts in there and add a few simple lazy state tests and then a test or two on this specifically?

Done

@doc-han doc-han requested a review from josephjclark December 6, 2024 13:31
@josephjclark
Copy link
Collaborator

Thanks @doc-han ! Appreciate it

@josephjclark josephjclark force-pushed the farhan/allow-function-calls-on-lazy-state branch from 290ecb9 to 17d32e2 Compare December 9, 2024 11:25
@josephjclark josephjclark merged commit f77a958 into main Dec 11, 2024
7 checks passed
@josephjclark josephjclark deleted the farhan/allow-function-calls-on-lazy-state branch December 11, 2024 15:10
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.

Lazy state fails on function calls
2 participants