Compiler: add default export even if there's an export already in the code (strict check for default export, not just /export/)

[doc-han]

Description.

Runtime seems to always require a compiled job to have a default export of an array. Hence, those without a default export break the runtime.

Because

1. we access the default export.
   

   kit/packages/runtime/src/execute/expression.ts

   Line 146 in 4376f7b

   const operations = exports.default;

2. we expect an array and want to iterate it
   

   kit/packages/runtime/src/execute/expression.ts

   Line 53 in 4376f7b

   ...operations.map((op, idx) =>

Currently,

We create a default export and then put all top-level-expressions in it. If there's no top-level-expression and empty array is exported to make the runtime happy.

export default []; // to make runtime happy!

But,

when there's an export (either named or not) that isn't a default export, we ignore the creation of the default export which makes the runtime sad.

There might be programs to be run in this job but because of a single non-default export. They don't get executed.

eg.

import {fn} from "@openfn/common"

fn(state => {
  state.data.name = "doc-han"
  return state;
})

export const han = "Han"; // prevents our runtime from running

fn(() => {
  state.data.isGithub = true;
  return state;
})

Is there a reason we don't accommodate this? @josephjclark
I feel we can have other exports and still generate our default export. Also, if the user provides a default export(which isn't encouraged) we can also check if it exports an Array.

Here we do a regex match.

kit/packages/compiler/src/transforms/ensure-exports.ts

Line 15 in 4376f7b

const currentExport = path.node.body.find(({ type }) => type.match(/Export/));

For the concern of having several types of export nodes. A default export seems to always have only one type of Node representation. The name of the node only changes across ast-generator.

Proposed solution.

1. Instead of regex matching the type 'Export ', Look for default export node instead.
2. check if the default export node has declaration that's an Array

[doc-han]

I don't even think that export there should be allowed. It's on the same level as

some-job.js

console.log("here")

because the export doesn't consume state and return state.

[josephjclark]

Hiya @doc-han

The default export is the basic contract between the a job and the runtime. I suppose you can look at it like this: the runtime will load a JS module that exports an array of functions as its default export. And then it'll execute those functions in series. It's a nice clean interface between a job and the rumtime.

There's no need for a job expression to export anything today.

Saying that, I would like it to support named exports so that we can unit test job code later on.

You're absolutely right - we should be doing a stricter check for export default rather than just that lazy regex.

But this isn't an issue I'm looking to fix in the short-medium term. We've got way more interesting problems to fix.

[josephjclark]

I'll rename this issue so that we can keep it in the backlog

[josephjclark]

@doc-han why do you want to add an export anyway?

[doc-han]

@doc-han why do you want to add an export anyway?

I actually don't want to.

1. I saw the regex match and tried it out.
2. I then saw a test guarding that behaviour.
   

   kit/packages/compiler/test/compile.test.ts

   Lines 13 to 18 in 4376f7b

   	test('do not add default exports if exports exist', (t) => {
   	const source = 'export const x = 10;';
   	const expected = 'export const x = 10;';
   	const result = compile(source);
   	t.is(result, expected);
   	});

[josephjclark]

@doc-han Got it - eagle eyes you have there 🦅