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

Have provider invoke fn support not being given a function name #776

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

jeffcharles
Copy link
Collaborator

Description of the change

Adds support to QuickJS provider's exported invoke function to handle not being given a function name. This shouldn't change any behaviour.

Why am I making this change?

As part of #768, we'll want to remove the eval_bytecode exported function (to make it easier for plugin developers) and just have invoke so invoke needs to also be able to handle eval_bytecode's use case where there is not an exported function to run.

I haven't deleted eval_bytecode in this PR because we still emit function invocations for it. I also haven't updated the dynamically linked codegen code to call invoke instead of eval_bytecode because all runtime environments using the provider would need to be updated with a provider version that includes this code change, otherwise new modules generated to use invoke with 0 arguments for function name pointer and length would fail.

Checklist

  • I've updated the relevant CHANGELOG files if necessary. Changes to javy-cli and javy-core do not require updating CHANGELOG files.
  • I've updated the relevant crate versions if necessary. Versioning policy for library crates
  • I've updated documentation including crate documentation if necessary.

Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

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

Left one minor comment -- not blocking, else LGTM.

Comment on lines 256 to 257
Invoke,
InvokeWithFn(&'static str),
Copy link
Member

Choose a reason for hiding this comment

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

Just a minor comment here - could this be consolidated into:

pub enum UseExportedFn {
  Invoke(Option<&'static str>)
  ...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I can make that change.

@jeffcharles jeffcharles merged commit 7fa858f into main Oct 7, 2024
7 checks passed
@jeffcharles jeffcharles deleted the jc.add-support-for-no-fn-to-invoke branch October 7, 2024 21:24
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.

2 participants