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

feat(#580): a basic implementation of js function calling from askcript #590

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pkarw
Copy link
Collaborator

@pkarw pkarw commented May 20, 2021

With this feature developers get a new helper/factory method which can be used for passing JS modules and functions to AskScript without the need of manually warping them with resources.

Usage:

import { factory } from 'askql/askvm/factory';
const moduleToBeWrapped = {
  wrapMe1: () => {
    return 100;
  },
  wrapMe2: (arg: string) => {
    return arg;
  },
  dontWrapMe: () => {
    return 0;
  },
};

function ask(code: string) {
  return runUntyped(
    {
      resources: {  ...factory(moduleToBeWrapped, ['wrapMe1', 'wrapMe2'])},
      values,
    },
    parse(code)
  );
}

// now you can call the function `wrapMe1` of `moduleToBeWrapped` in AskScript
ask(`ask(call(get('wrapMe1')))

You can easily wrap a whole module like lodash by just simply:

  return runUntyped(
    {
      resources: {  ...factory(lodash, ['sort'])},
      values,
    },
    parse(code)
  );

lukasz-xfaang
lukasz-xfaang approved these changes May 24, 2021
Copy link
Collaborator

@czerwinskilukasz1 czerwinskilukasz1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

);
});
it(`should wrap wrapMe2 js method nad return arg`, async function () {
const expectedResult = 'arg';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe using a different string than 'arg' would be better, so that it's not the same as the variable name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@czerwinskilukasz1
Copy link
Collaborator

@mhagmajer , do you have any specific comments on this PR?

@pkarw , I added a few minor comments if you had time, but feel free to merge it without addressing them.

@pkarw pkarw requested a review from mhagmajer June 17, 2021 11:08
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

Successfully merging this pull request may close these issues.

4 participants