CLI: Simplify handling of imports

[josephjclark]

Short Description

Adds a dumb-imports option to the CLI by default.

This should enable nested imports like common.http, common.beta and mailchimp.md5 to work.

Related issue

This is a workaround to #238, but closes #363 and the mailchimp md5 issue over in adaptors

Implementation Details

The CLI currently fails for jobs which reference any re-exported module from an adaptor. For example common exports http, beta, dateFns. All of these aliased exports (or nested namespaces?) will result in a Reference Error in the new CLI.

For any job, we generate an import statement - eg, import { fn, http } from '@openfn/language-common'. To ensure this import statement is accurate, we look at the actual exports of the adaptor and try to only import what we need.

The problem, which is broadly captured in #238, is that we are failing to get an accurate list of an adaptors exports. This mean that for http and other exports, no export is reported and so no import is generated. This results in a runtime exception because we're referencing an undefined symbol.

To fix this properly we need to dig deeply into describe-package. Actually, we need to basically re-write it - see #197.

This fix basically say: don't bother to look up the adaptor's actual imports, just assume that all undeclared variables are imported from the adaptor (apart from globals).

The risks of this change are:

• If you don't declare a variable properly in job code (ie, x = 10), the compiler will and import that variable from the adaptor. This will result in a runtime error. This is probably quite likely to happen.
• If you reference a global variable that isn't on our list, we'll try and import it from the adaptor, resulting in an error. This is unlikely (and easy to fix)

QA Notes

It would be helpful to re-test these issues (and close them, if they're not already)

• CLI: ReferenceError: dateFns is not defined #363
• mailchimp bug where md5 cannot be used in the new CLI adaptors#383
• describe-package: handle namespaces in export descriptions #80 (see the beta stuff)

Checklist before requesting a review

• I have performed a self-review of my code
• I have added unit tests
• Changesets have been added (if there are production code changes)

[taylordowns2000]

@josephjclark , this feels like an improvement as it allows the beta.each and http.blah stuff that we'll need to support from v1.

i see it's only in the CLI, should we be handling this in the worker/runtime also?

[josephjclark]

@taylordowns2000 Thank you for taking a look!

In my head this was going to benefit the worker for free - but you're right, it only affects the CLI right now.

I will add this to the worker in time for the next release. It's a bit of a scary change as it runs so deep (but it's not half so scary as a "full" fix in the Typescript compiler)

[josephjclark]

@taylordowns2000 The other strategy is, as the Worker is about to come under loads of stress testing anyway, that we just raise this for the Worker and leave the CLI alone.

We'll soon know if it's a net benefit or not.

[josephjclark]

By the way I 100% guarantee that this approach will cause problems. It will for sure lead to misleading error messages and might catch out users who are doing weird stuff with global variables.

The question is whether it'll fix more than it breaks!

[mtuchi]

@josephjclark I am happy to report that http.get() is working as expected when using salesforce adaptors, But i noticed this bug ReferenceError: sheetToBuffer is not defined, during testing 👇🏽

 openfnx job.js -ma msgraph -s ms-state.json         
[CLI] ♦ Versions:
         ▸ node.js                     18.12.1
         ▸ cli                         branch/fix-compiler-imports
         ▸ runtime                     ./openfn-runtime-0.0.31-local.tgz
         ▸ compiler                    ./openfn-compiler-0.0.36-local.tgz
         ▸ @openfn/language-msgraph    latest
[CLI] ✔ Loading adaptors from monorepo at /Users/openfn/Workspace/adaptors
[CLI] ✔ Loaded state from ms-state.json
[CLI] ✔ Compiled from job.js
[R/T] ♦ Starting job job-1
vm:module(0):1
export default [sheetToBuffer(
^

ReferenceError: sheetToBuffer is not defined
    at vm:module(0):1:1
    at SourceTextModule.evaluate (node:internal/vm/module:226:23)
    at module_loader_default (file:///Users/openfn/.asdf/installs/nodejs/18.12.1/lib/node_modules/@openfn/clix/node_modules/@openfn/runtime/dist/index.js:286:16)
    at async prepareJob (file:///Users/openfn/.asdf/installs/nodejs/18.12.1/lib/node_modules/@openfn/clix/node_modules/@openfn/runtime/dist/index.js:397:21)
    at async file:///Users/openfn/.asdf/installs/nodejs/18.12.1/lib/node_modules/@openfn/clix/node_modules/@openfn/runtime/dist/index.js:362:35

Node.js v18.12.1

Here are some code to reproduce it

sheetToBuffer(
  [
    {
      name: {
        first: 'John',
        last: 'Adams',
      },
    },
  ],
  {
    wsName: 'Some Report',
    bookType: 'csv',
  }
);

Side Note

Looking at the adaptor code i noticed that sheetToBuffer and uploadFile are exported from msgraph/Util.js - https://github.com/OpenFn/adaptors/blob/main/packages/msgraph/src/Adaptor.js#L370

Could that be the reason ?

[josephjclark]

Thank you @mtuchi! That's really helpful. I'll investigate and get back to you.

Presumably the export is something to do with the issue - although really this fix should resolve it, in exactly the same way that http gets resolved. I'll have to dig into it.

[mtuchi]

Hiya @josephjclark , Any update on this PR ?

Thank you @mtuchi! That's really helpful. I'll investigate and get back to you.

Presumably the export is something to do with the issue - although really this fix should resolve it, in exactly the same way that http gets resolved. I'll have to dig into it.

[mtuchi]

I have also noticed if your using a common adaptor you can't use get() or post(). You will get an error

{"message":"SyntaxError: The requested module '@openfn/language-common' does not provide an export named 'get'","name":"RuntimeCrash","severity":"crash","source":"runtime","subtype":"SyntaxError","type":"RuntimeCrash"}

to resolve that i had to manually export get() from @openfn/language-common

import { http } from "@openfn/language-common";
const { get } = http

get({
  url: "https://jsonplaceholder.typicode.com/users",
  headers: {
    "content-type": "application/json"
  }
})

[josephjclark]

No update @mtuchi . It's on the list, but I'm not likely to get to it this month.

Whatever solution we adopt needs to be rolled out in CLI and the Worker, so it's quite a bit of work.

[josephjclark]

I am tempted to port this over to 1.0, although it's fairly low priority (and a bit risky still) so I may patch it in after

[mtuchi]

@josephjclark do you still need me to test this ?

[josephjclark]

@mtuchi Not at the moment, thank you! I'll try and get it sorted soon though

[josephjclark]

Closing as this is properly fixed by #652