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

Refactor the structure of the Config/APIs #646

Merged

Conversation

saulecabrera
Copy link
Member

@saulecabrera saulecabrera commented May 9, 2024

This change primarily rewrites the current APIs using "Intrinsics" as defined by rquickjs.

This change has several objectives:

  • Refactor the structure of the Config and its relationship with the Runtime to pave the way for a truly extensible and configurable engine. The way Intrinsics are exposed in rquickjs simplifies (i) enabling a granular configuration experience of the native intrinsics, which was not entirely clear how to achieve before, and (ii) adding custom APIs as intrinsics.

  • Enhance how our configuration operates: the previous model assumed an almost static configuration setup, allowing only limited, non-straightforward customizability from the CLI in the future. This was mostly modeled per API and not as a global aspect of the runtime. Our recommended approach for users needing functionality beyond what was provided was to fork or recreate their own version of the CLI/APIs. A telling sign of this issue is that the configuration parameters were almost never used in any of the APIs. This PR paves the way for greater extensibility, which will be facilitated by threading configuration options from the CLI in a follow-up PR; this method will also allow for opting-in to non-standard functionality via a CLI flag, helping us avoid issues like the one noted here: Allow specifying which file descriptor is used in console.{log,err} from the CLI #628.

Along the way, I had to make several changes to make all this work; the most relevant ones are: (i) deprecating the javy-apis crate (ii) enabling all the APIs by default and (iii) reworking a bit the Console implementation.

The reasoning to deprecate the javy-apis crate is mostly around: (i) the fact that we're revamping our extensilbity model in the near fuiture and (ii) the complexity around cyclical dependencies: javy exposes rquickjs and given that the cofiguration is tied to the runtime, javy needed to depend on javy-apis but the other way around was also true. I decided not to spend too much cycles on this mostly given point (i) above.

There are still things to follow-up on after this PR, namely

  • Update all the documentation under docs
  • A PR to thread the engine configuration from the CLI. NB that the engine behaviour is not altered in this PR.

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.

This change primarily rewrites the current APIs using "Intrinsics" as defined by rquickjs.

This change has several objectives:

Refactor the structure of the Config and its relationship with the Runtime to pave the way for a truly extensible and configurable engine. The way Intrinsics are exposed in rquickjs simplifies (i) enabling a granular configuration experience of the native intrinsics, which was not entirely clear how to achieve before, and (ii) adding custom APIs as intrinsics.
Enhance how our configuration operates: the previous model assumed an almost static configuration setup, allowing only limited, non-straightforward customizability from the CLI in the future. This was mostly modeled per API and not as a global aspect of the runtime. Our recommended approach for users needing functionality beyond what was provided was to fork or recreate their own version of the CLI/APIs. A telling sign of this issue is that the configuration parameters were almost never used in any of the APIs. This PR paves the way for greater extensibility, which will be facilitated by threading configuration options from the CLI in a follow-up PR; this method will also allow for opting-in to non-standard functionality via a CLI flag, helping us avoid issues like the one noted here: bytecodealliance#628.

Along the way,  I had to make several changes to make all this work; the most relevant ones  are: (i)  deprecating the `javy-apis` crate (ii) enabling all the APIs by default and (iii) reworking a bit the `Console` implementation. 

The reasoning to deprecate the `javy-apis` crate is mostly around: (i) the fact that we're revamping our extensilbity model in the near fuiture and (ii)  the complexity around cyclical dependencies:  `javy` exposes rquickjs and given that the cofiguration is tied to the runtime, `javy` needed to depend on `javy-apis` but the other way around was also true. I decided not to spend too much cycles on this mostly given point (i) above. 

There still things to follow-up on after this PR, namely 

- [ ] Update all the documentation under  `docs` 
- [ ] A PR to thread the engine configuration from the CLI. NB that the engine behaviour is not  altered in this PR.
@saulecabrera saulecabrera requested a review from surma May 9, 2024 19:42
@saulecabrera
Copy link
Member Author

cc/ @surma since we chatted about using Intrinsics, I decided to give this a try as part of adding a faster JSON implementation.

@saulecabrera saulecabrera requested a review from jeffcharles May 9, 2024 19:43
Copy link
Collaborator

@jeffcharles jeffcharles left a comment

Choose a reason for hiding this comment

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

Do we plan to remove the stream I/O and console APIs into a plugin later after we've figured out a common extension mechanism?

crates/core/Cargo.toml Outdated Show resolved Hide resolved
crates/javy/src/apis/console/mod.rs Outdated Show resolved Hide resolved
crates/javy/src/apis/console/mod.rs Outdated Show resolved Hide resolved
crates/javy/src/runtime.rs Outdated Show resolved Hide resolved
@saulecabrera
Copy link
Member Author

Do we plan to remove the stream I/O and console APIs into a plugin later after we've figured out a common extension mechanism?

...and the soon-to-be-introduced JSON functions. That would be the ideal outcome I believe, given that they introduce non-standard functionality and it will also allow us to test our extension mechanism.

@saulecabrera saulecabrera requested a review from jeffcharles May 14, 2024 18:07
@saulecabrera saulecabrera merged commit 30c21d0 into bytecodealliance:main May 14, 2024
14 checks passed
@saulecabrera saulecabrera deleted the refactor-api-structure branch May 14, 2024 18:22
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