Skip to content

CLI API in @polywrap/test-env-js#1343

Closed
krisbitney wants to merge 4 commits into
origin-devfrom
test-env-js-cli-api
Closed

CLI API in @polywrap/test-env-js#1343
krisbitney wants to merge 4 commits into
origin-devfrom
test-env-js-cli-api

Conversation

@krisbitney
Copy link
Copy Markdown
Contributor

@krisbitney krisbitney commented Oct 18, 2022

This PR adds a set of methods that can be used to programmatically invoke the CLI. These are largely based on existing methods, but are generalized for broader application and increased flexibility.

I think they could be useful, because they are based on my experience. The ideas came from methods I created and used for testing wrappers in the past.

  • infraUp - starts infra based on modules declared in infra manifest
  • infraDown - stops infra based on modules declared in infra manifest
  • deploy - deploys wrapper based on deploy manifest
  • build - same as buildWrapper but with more options
  • codegen - executes codegen for a wrapper

I haven't yet written formal tests for the methods because I want to get feedback before moving forward with this. Is it a good idea? Should the api be changed, moved, refactored?

I did not remove the existing methods from the @polywrap/test-env-js package, but I renamed buildWrapper to build. I would like to refactor them to use the new methods "under the hood" and keep them as convenience methods, but I would like feedback first.

@krisbitney krisbitney requested a review from dOrgJelli as a code owner October 18, 2022 16:45
@krisbitney krisbitney marked this pull request as draft October 18, 2022 16:45
@pileks
Copy link
Copy Markdown
Contributor

pileks commented Oct 18, 2022

The one thing that comes to mind here is that we might want to have all, or at least a majority of our CLI commands accessible through some kind of abstraction.

What comes to mind is mainly the maintainability of the e2e tests - a naive example would be changing the name of a single command wherein we could simply change its name in this abstraction layer instead of searching through tests and changing them manually.

In any case, this seems super-helpful, I'm all for it 👍

@dOrgJelli
Copy link
Copy Markdown
Contributor

dOrgJelli commented Oct 18, 2022

I agree that we need something like this, but think adding it to the "test-env" package isn't ideal. The test-env package itself is already a pretty legacy package, and should be removed eventually as it doesn't make much sense IMO.

Starting at a high level, I think it makes sense to be able to import the CLI as an NPM package, enabling easy programatic usage. This can be done by having both a bin and a main within the package.json like so:

{
  "name": "polywrap",
  "description": "Polywrap CLI",
  ...
  "bin": {
    "polywrap": "bin/polywrap"
  },
  "main": "build/index.js",
  ...
}

This will allow users to run npx polywrap ... on the command line which will utilize the bin entry point, and import the CLI as a JS module like so:

import { ... } from "polywrap"

Having this within the CLI's package will allow us to reuse types, like the command's options.

As for the devexp for the "polywrap" package, I think something like this would be good:

import { Commands } from "polywrap";

const results = Commands.build.run({
  typedOptions: "foo"
});

// .run(...) spawns a child process, and doesn't execute in-process

results.stdout
results.stderr
results.exitCode

EDIT: one last thought, I think this looks a bit weird from "polywrap";, and would much prefer something like from "@polywrap/cli". Maybe the @polywrap/cli package is a wrapper around the "polywrap" package that's used for programatic execution, but the "polywrap" package is the CLI itself, named nicely for execution via npx polywrap ....

@pileks
Copy link
Copy Markdown
Contributor

pileks commented Oct 18, 2022

I agree fully with @dOrgJelli, it looks like a big step toward giving our CLI the TLC it deserves, and it won't be a "circumvent fix" like the one I proposed.

If you @krisbitney and @dOrgJelli agree, I'd be willing to open an issue and start working on this.

@krisbitney
Copy link
Copy Markdown
Contributor Author

krisbitney commented Oct 19, 2022

I agree fully with @dOrgJelli, it looks like a big step toward giving our CLI the TLC it deserves, and it won't be a "circumvent fix" like the one I proposed.

If you @krisbitney and @dOrgJelli agree, I'd be willing to open an issue and start working on this.

I think it sounds like a good plan but I would like to continue working on it. Maybe we can work together on it?

@pileks
Copy link
Copy Markdown
Contributor

pileks commented Oct 19, 2022

@krisbitney All yours, feel free to hit me up whenever you want an additional set of hands on it!

@krisbitney krisbitney closed this Oct 20, 2022
@krisbitney krisbitney deleted the test-env-js-cli-api branch February 7, 2023 17:12
@krisbitney krisbitney restored the test-env-js-cli-api branch February 7, 2023 17:12
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.

3 participants