OpenFn / kit

The bits & pieces that make OpenFn work. (diagrammer, cli, compiler, runtime, runtime manager, logger, etc.)
10 stars 9 forks source link

Lazy State Operator #637

Closed josephjclark closed 6 months ago

josephjclark commented 6 months ago

Overview

Add support for a lazy-state operator $, which allows state references to be lazily evaluated.

get($.data.url)

It should be a straight replacement for any dataValue() call, or any simple (state) => state.data.x reference.

Issue

This is a great solution to https://github.com/OpenFn/adaptors/issues/483

Details

Any reference to JSON-path like expressions like$.a.b.c will be compiled into arrow references, like (state) => state.a.b.c. This is syntactic sugar to make it easier to safely pass values from state into operations.

Examples

From the doc site. This:

create(
  'Patient__c',
  fields(
    field('Name', dataValue('form.surname')),
    field('Other Names', dataValue('form.firstName')),
    field('Age__c', dataValue('form.ageInYears')),
    field('Is_Enrolled__c', true),
    field('Enrollment_Status__c', 3)
  )
);

Becomes this:

create(
  'Patient__c',
  fields(
    field('Name', $.data.form.surname),
    field('Other Names', $.data.form.firstName),
    field('Age__c', $.data.form.ageInYears),
    field('Is_Enrolled__c', true),
    field('Enrollment_Status__c', 3)
  )
);

Or even this:

create(
  'Patient__c',
  {
    'Name':  $.form.surname,
    'Other Names': $.data.form.firstName,
    'Age__c': $.data.form.ageInYears,
    'Is_Enrolled__c': true,
    'Enrollment_Status__c': 3,
  }
);

TODO

This is a very basic implementation of this feature. Probably good enough to release as an alpha feature.

There is very little intelligence, robustness or error handling on this. It is brittle.

At the moment, any $a.b.c reference will be converted. If you do const $ = {}; get($.a.b.c), the compiler will still convert the dollar reference.

Probably we should only do this on arguments to operations, maybe even only at the top level (so that fn((state) => get($.data.url)(state)) would fail). But it'll all be compiled right now and will likely break in job code.

Maybe I could print a console warning that this is an experimental feature :thinking:

josephjclark commented 6 months ago

@mtuchi Are you up for a little play with this? Happy to talk you through it if the above is unclear

CC @taylordowns2000 also!

mtuchi commented 6 months ago

@josephjclark If the compiler will compile $.a.b.c to state => state.a.b.c do i need adaptor functions at all ? Can i just write a generic js function that manipulate state like this 👇🏽

const [firstName, lastName] = $.data.name.split(' ');
$.fullName = `${firstName} ${lastName}`;

Can i test this by running pnpm install:openfnx and use openfnx path/job.js ?

josephjclark commented 6 months ago

@mtuchi:

  1. Yes, you still need an adaptor*
  2. That code snippet won't really do anything
  3. You can't assign back to state with this - it's just a shortcut to read state
    1. Yes, you can test in openfnx
josephjclark commented 6 months ago

I'm parking this today before I go afk for a week.

We could release it as-is, without documentation, as a secret feature. That would give folks a bit of time to test it out and see how it feels, work out the edge cases.

Before we can release it publicly, we need:

josephjclark commented 6 months ago

@taylordowns2000 no problem, the optional chaining thing is sorted :+1:

I'm gonna release all this now in a minute

mtuchi commented 6 months ago

@josephjclark i noticed parseXML being affected by this change. I am getting this error [R/T] ✘ TypeError: TypeError: inner is not a function This is how i am using parseXML

parseXML(state.data, $ => {
    const string = ($, elementName) => $.find(elementName).text();
    const response = $('response');
    console.log(response);
    const inner = $.load(response.text());

    const result = [];

    // iterate over each line element in the dataset
    inner('line').each((i, el) => {
      // Get a cheerio selector on the line element
      const $line = $(el);
      // build a salesforce record
      const record = {
        code: string($line, 'AccountCode'),
        period: string($line, 'AccountingPeriod'),
        amount: string($line, 'baseamount'),
      };

      result.push(record);
    });

I think the error happens when we use $.load() function

josephjclark commented 6 months ago

Thanks, @mtuchi, I think I see the problem

I'll look at a fix next week

As a workaround, try not using $ as the name of your Dom selector thingy. Maybe just rename it to doc, like this:

parseXML(state.data, doc => {
  //...
})