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

Expose the request to cicero-engine.init() #604

Closed
discochance opened this issue Dec 3, 2020 · 6 comments · Fixed by #620
Closed

Expose the request to cicero-engine.init() #604

discochance opened this issue Dec 3, 2020 · 6 comments · Fixed by #620

Comments

@discochance
Copy link

Is your feature request related to a problem? Please describe.
A the moment, init sets the parameters to {} in https://github.com/accordproject/cicero/blob/b78e759d47053f73f9d7a5d357b4ace98807da40/packages/cicero-engine/lib/engine.js#L85 but for certain use-cases, it is necessary to pass an object to the init clause.

Describe the solution you'd like
The interface could expose the object like init(clause, currentTime, request = {}), it would be possible to pass additional arguments.

Additional context
I have created a minimal example from the HelloWorld Clause helloworld.zip. This still fails when I test modifying the passed object locally with:

node:internal/process/promises:225
          triggerUncaughtException(err, true /* fromPromise */);
          ^
TypeError: Cannot use 'in' operator to search for '$class' in null
    at unbrand (vm.js:547:22)
    at orgXaccordprojectXhelloworldXHelloWorld.init (vm.js:84:109)
    at vm.js:3:30
    at Script.runInContext (node:vm:143:18)
    at VM.run (/local/app/node_modules/vm2/lib/main.js:211:72)
    at VMEngine.runVMScriptCall (/local/app/node_modules/@accordproject/ergo-engine/lib/vmengine.js:80:19)
    at VMEngine.invoke (/local/app/node_modules/@accordproject/ergo-engine/lib/engine.js:185:29)
    at VMEngine.init (/local/app/node_modules/@accordproject/ergo-engine/lib/engine.js:216:21)
    at Engine.init (/local/app/node_modules/@accordproject/cicero-engine/lib/engine.js:85:32)
    at test (/local/app/app.js:51:31)
@imSanko
Copy link

imSanko commented Dec 23, 2020

I wanna work on this project can you assign it to me.

@jeromesimeon
Copy link
Member

Thanks for the report @discochance. After some review it seems that this was always something supported by the engine, but simply not exposed in the API.

I have a PR #620 open to allow cicero initialize (in the CLI) and Engine.init(...) with initialization parameters.

Feedback welcome.

@jeromesimeon
Copy link
Member

@irmerk I really dislike automatic closing of issues. We should give maintainers and issue originators the chance to further comment before closing.

@jeromesimeon
Copy link
Member

@discochance A fix is in! Let us know if this seems satisfactory or if any further questions.

@discochance
Copy link
Author

Thank you very much, this is exactly what we needed!

@jolanglinais
Copy link
Member

@jeromesimeon the PR template auto links (and thus auto closes) the associated issue which is placed in L4 (# Closes #<CORRESPONDING ISSUE NUMBER>). This can be changed by the author to be something not on this list so GitHub doesn't auto link them but there is a reference to the Issue. So it could be changed to # Solves #<ISSUE> and it shouldn't link it. Also you can unlink the Issue regardless in the PR before merging it and it won't auto close the Issue.

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

Successfully merging a pull request may close this issue.

4 participants