refactor(cli): Librarify nova_cli to avoid code duplication
#916
Conversation
niluxv
changed the title
nova_cli to avoid code duplicationnova_cli to avoid code duplication
niluxv
force-pushed
the
librarify-cli
branch
from
262a806 to
5bcfde9
Compare
This already factored out some logic that was duplicated between the `eval` and `repl` subcommands. However, the real goal is to provide a library the benchmark runner can use, to avoid code duplication there.
niluxv
force-pushed
the
librarify-cli
branch
from
5bcfde9 to
0dbdc3a
Compare
aapoalas
left a comment
aapoalas
left a comment
There was a problem hiding this comment.
This looks good to me: I don't unfortunately have any particularly good ideas about what a more generic API would look like. Storing parsed-but-not-executed Scripts could indeed be done using Global<Script>s, but I don't have a handle on if that would work nicely for a benchmarking CLI or not.
I'd be happy to merge this as-is; I kind of expect that figuring out the correct API is something that can only be done through experiment.
nova_cli/src/lib/host_hooks.rs
Outdated
| ) { | ||
| let specifier = module_request.specifier(agent); | ||
| let specifier = specifier.to_string_lossy(agent); | ||
| let specifier_target = if let Some(specifier) = specifier.strip_prefix("./") { |
There was a problem hiding this comment.
thought: I wonder if some of this code would belong in the ModuleMap now?
There was a problem hiding this comment.
Oh, good catch. Yes, that makes a lot of sense.
There was a problem hiding this comment.
I forgot to say: sorry it took so long for me to review this <3
There was a problem hiding this comment.
Absolutely no problem, I was busy as well so it was a blessing, not a curse ;)
niluxv
changed the title
nova_cli to avoid code duplicationnova_cli to avoid code duplication
|
Ok, indeed we can further experiment after this is merged; the no semver guarantee gives us flexibility there. And it would be quite a nightmare to merge/rebase this if someone makes significant changes to the cli code in the meantime... How about I rebase this on top of main so we lose the merge commit, and squash my two commits together? |
Sounds good to me! |
3 participants
Right now this is mostly moving code around, so the benchmark runner (which I have in a separate branch) can share most of its code with the cli. I have no idea whether this library could be useful for users of nova as well, I don't really know their requirements, but I assume the current setup is not flexible enough for that. (For example, currently everything assumes there is only one realm.)
The benchmark runner needs to separately parse and execute scripts, while the cli can just immediately execute a parsed script, which is why I haven't touched the script parsing and execution logic of the cli. However, maybe we can store a list of parsed but not yet executed scripts in
HostDefined(alongside the module map of already executed modules, or maybe merge them?), and then implement convenience commands for script/module parsing and execution for theInstance. Or maybe that is overly complicated and it is better to leave this work to the library user.I would love to hear any suggestions, as I find myself really struggling to provide a flexible and convenient API.