Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Migrated code generation logic to it's own crate #890

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ianmarmour
Copy link
Contributor

@ianmarmour ianmarmour commented Feb 16, 2025

Description of the change

Splits Code Generation logic out into it's own crate (from CLI).

Things I attempted to do pro-actively here,

  • Remove unnecessary dependencies from both the javy-codegen crate and the cli crate.
  • Added documentation and ensured all public interfaces are documented in javy-codegen.
  • Re-exported commonly used structs for ease of use.

Things we might want to change/add,

  • Crate specific tests ? (Maybe)
  • ???

Why am I making this change?

To enable runtime generation of WASM modules without shelling out to a CLI.

Checklist

  • I've updated the relevant CHANGELOG files if necessary. Changes to javy-cli and javy-plugin 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.

Related PR's

877, 879

@ianmarmour ianmarmour marked this pull request as ready for review February 16, 2025 00:32
@jeffcharles jeffcharles mentioned this pull request Feb 18, 2025
3 tasks
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.

Can we add the crate feature we talked about before so we can reduce visibility for the methods for working with the v2 and default plugins to the CLI crate?

Otherwise, I think it's a structurally sound change besides removing some methods from the public API.

@jeffcharles jeffcharles mentioned this pull request Feb 18, 2025
3 tasks
@ianmarmour
Copy link
Contributor Author

Can we add the crate feature we talked about before so we can reduce visibility for the methods for working with the v2 and default plugins to the CLI crate?

Otherwise, I think it's a structurally sound change besides removing some methods from the public API.

Totally forgot, of course!

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.

I noticed a few more things while looking at this today.

While the CLI integration tests provide some level of test coverage, I feel like those tests are not testing at least a couple default behaviours of the generator. So might be good to write some tests to ensure those behaviours are what we want and aren't changed unintentionally.

I imagine at some point in the future, it might make sense to migrate a bunch of the integration tests to use this crate instead of using the CLI which should help with more direct test coverage but it doesn't make sense to do that in this PR.

@@ -0,0 +1,33 @@
[package]
name = "javy-codegen"
version.workspace = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we set this to "1.0.0-alpha.1" for now?

I want to version this crate independently of the CLI and javy library crate. The programmatic interface and default behaviour of the CLI can change independently of the API of this crate so I'd prefer not to couple the versions together.


// Generate your WASM module.
let wasm = generator.generator(&js)?;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we put this in the module docs as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should add documentation for the Cargo features here just saying they're not for public use and the APIs are not stable. I was thinking similar to how things are documented in the javy library crate. It would also be good to add a core concepts section.


// Generate your WASM module.
let wasm = generator.generator(&js)?;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also add an automated integration test for this example code in this crate?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the behaviour if the consumer does not set a plugin before calling generate? Also can we add a test for this case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the behaviour if the consumer does not set set_compression? On the CLI, we've generally compressed the source code by default.

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

Successfully merging this pull request may close these issues.

2 participants