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

Allowing adaptors to extend execute #22

Closed
josephjclark opened this issue Sep 22, 2022 · 3 comments
Closed

Allowing adaptors to extend execute #22

josephjclark opened this issue Sep 22, 2022 · 3 comments

Comments

@josephjclark
Copy link
Collaborator

josephjclark commented Sep 22, 2022

Some adaptors, like postgres, allow the execute function to be extended.

This will break in v2 at the moment!

What's really going on here is the adaptor wants to add setup and teardown hooks which run at the start and end of the pipeline.

I think the solution is to publish a new adaptor which is only compatible with the new runtime. The adaptor would look like this:

import { addPrePipelineHook, addPostPipelineHook } from '@openfn/runtime';

function setup(state) { ... }
function teardown(state) { ... }

addPrePipelineHook(setup)
addPostPipelineHook(setup)
export { setup, teardown, connect, ... }

Whenever the adaptor is imported, it'll automatically run the setup and teardown operations at the start and end of the pipeline. it's similar to the overridden execute really, but a bit less magical.

What I like about this approach is that there's no compiler or runtime magic. v1 jobs will continue to work in the new runtime.

The only downside I can see is that the next-gen adaptor will fail in the old engine. Given that the platform can control which adaptors are loaded, I think this is probably acceptable?

It also gets a bit sticky in he new CLI because you'll need to target the CLI at the correct adaptor version. Again this is probably OK, but may rely on a good versioning strategy eg openfn . -a @open/language-postgres@2.0.0

@josephjclark
Copy link
Collaborator Author

After thinking about this, I think it's unacceptable that old adaptors should be totally incompatible with the new runtime.

I think the compiler should add the magic to export an execute function. The runtime will use the job's exported execute, if it exists (and print a warning about it).

Which means automated imports probably need to look something like this:

import * as adaptor from '@openfn/language-postgres';
const { get, put, execute } = adaptor;
export execute; // is that even legal if adaptor is undefined?

export default[get(state.url)];

Later, we can add intelligence to only do this for language-adaptors which we know override execute. Although this relies on the adaptor having a .d.ts really, so maybe we invert the logic and do this unless a) the package has a .d.ts and b) the d.ts does not export execute).

New adaptors should still register side-effects from @openfn/runtime on import.

@josephjclark
Copy link
Collaborator Author

Hang on, a way easier fix is just to export everything:

import { upsert } from "@openfn/language-postgres";
export * from "@openfn/language-postgres";
export default [upsert(state.data.incoming]);

@josephjclark
Copy link
Collaborator Author

Ok, so live on v2 at the moment we have a working strategy for legacy adaptors.

If exportAll is truthy on compilerOptions['add-imports'].adaptor, then an export default statement is added for the adaptor. This is always enabled in the CLI, so jobs compiled through the CLI will always export * from adaptor.

If a job exports a function called execute, the runtime will use that function instead of its built-in one.

Next steps are to implement the lifecycle features described in #25.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant