feat(cli): create cli package with config, plugins and commands #6

thymikee · 2024-09-26T14:07:35Z

Summary

First draft of a very simple config system that loads plugins defined as:

function Plugin(config: ConfigType) {
  return {
    name: "name",
    commands: [] // commands to add to the final config
  }
}

and registers the commands using commander for now. In the future I'd like to use a lighter tool, but let's start with something simple.

Test plan

Added unit tests with some helpers that I'll need to move to a separate package (done in #7) I suppose to not validate module boundaries

mdjastrzebski

I've left some questions

mdjastrzebski · 2024-09-26T16:52:44Z

packages/cli/tsconfig.spec.json

+    "src/**/*.test.ts",
+    "src/**/*.spec.ts",
+    "src/**/*.d.ts"
+, "__tests__/getConfig.spec.ts"  ]


Use pattern here.

Do we want to use top-level __tests__ mirroring the src folder structure or put __tests__ folder in the src tree, next to the actual code?

This was autogenerated by Nx I guess. I think it makes more sense to put __tests__ in src

+1 to __tests__ side by side the source, alternatively x.test.ts convention is also nice

mdjastrzebski · 2024-09-27T10:14:51Z

packages/cli/package.json

@@ -0,0 +1,18 @@
+{
+  "name": "@rnef/cli",


[nit] @callstack/rnef-cli?

mdjastrzebski · 2024-09-27T10:17:16Z

packages/cli/jest/helpers.ts

+ *   }
+ * );
+ */
+export const writeFiles = (


General questions, should we use sync io or rather opt for async/promise io to be able to display some fancy spinners (you need async io for that, right?)

Sync file access is generally faster in test environments so I prefer that

mdjastrzebski · 2024-09-27T10:28:48Z

packages/cli/src/lib/cli.ts

+  .version(version);
+
+export const cli = async () => {
+  const config = await getConfig();


How do we plan to handle case when two plugins define the same command? Last one wins or some error/warning to the user?

Initially I thought that for now it's ok that last one wins and getConfig eventually warns about 2 duplicate command names. I was also thinking about overriding mechanism like this:

module.exports = { plugins: { pluginAndroid, pluginApple, android2: otherAndroidPlugin, // <-- when defined this way we could prefix commands with `android2:` commands: { // <-- if we keep the commands in user config then this could also be possible [pluginAndroid.commands.build]: CustomCommand } } }

When we last discussed the pom configuration mechanism, you expressed some concerns about non-deterministic nature of arrays. In other words, the result being dependent on the order configs were loaded. How does that concern apply to this?

(I generally think it is hard to solve)

mdjastrzebski · 2024-09-27T10:29:36Z

packages/cli/src/lib/getConfig.ts

+type ConfigType = {
+  projectConfig?: object;
+  plugins?: Record<string, PluginType>;
+  commands?: Array<{


[nit] extract CommandType

mdjastrzebski · 2024-09-27T10:30:16Z

packages/cli/src/lib/getConfig.ts

+  commands?: Array<{
+    name: string;
+    description: string;
+    action: (config: ConfigType) => void;


[nit^2]

Suggested change

action: (config: ConfigType) => void;

run: (config: ConfigType) => void;

mdjastrzebski · 2024-09-27T10:32:38Z

packages/cli/src/lib/getConfig.ts

+
+const extensions = ['.js', '.ts', '.mjs'];
+
+const importUp = async <T>(dir: string, name: string): Promise<T> => {


So this looks up to find the first rnef.config? Do we want to support multi-level ones? Monorepo, etc?

I would assume you have a single rnef.config sitting in your monorepo root, or where RN is located. Since it's a JS file you could import project-specific config from other locations, if that's what you're after

This is interesting question and I initially had a different approach (sitting at the project level, stop when you hit first one) and I don't think that both are correct.

The problem here is when you have multiple React Native applications, or you want to customize behavior on per package basis. I think eventually we may need both.

For example, you have one application where you want to load different set of plugins (e.g. for brownfield), and the other one, being your greenfield sandbox, you want different set.

Again, this is likely an edge case scenario, but the idea was to make all the options possible w/o the CLI design being the limitation here.

mdjastrzebski · 2024-09-27T10:33:50Z

packages/cli/src/lib/getConfig.ts

+type ConfigType = {
+  projectConfig?: object;
+  plugins?: Record<string, PluginType>;
+  commands?: Array<{


q: Do we expect user to define their own commands in the config?

Up to us. For now I allow for this, but making a plugin is so simple, to we could also only allow making commands through plugins to simplify our config handling

+1 for requiring use to make a simple plugin

mdjastrzebski · 2024-09-27T10:34:17Z

packages/cli/src/lib/getConfig.ts

+};
+
+type ConfigType = {
+  projectConfig?: object;


What is the purpose of this?

empty placeholder for now. I was thinking we could put some generic platform-independent project config here, like root of the project, or RN path, but it doesn't have to be here

[nit] add code comment to descibe this intent

okwasniewski · 2024-09-27T10:42:29Z

packages/cli/src/lib/cli.ts

+      .description(command.description || '')
+      .action(async () => {
+        try {
+          await command.action(config);


Should we pass here the full config?

I guess the most useful for a plugin would be to get the config.projectConfig because the rest is just plugins/commands?

We should definitely strip the config to essentials. What I was also considering is adding some helpers like config.api.registerCommand instead of returning the commands as an output, although I picked to latter for now due to simplicity

packages/cli/eslint.config.js

grabbou · 2024-09-27T14:30:18Z

packages/cli/package.json

+    "tslib": "^2.3.0"
+  },
+  "devDependencies": {
+    "jest-util": "^29.0.0"


This is more of a general question, but why do we use jest over something such as vitest? Works better with TypeScript, ESM (for the future), and has a nice workspace-mode that allows running these faster.

I'm fine with vitest, ship it. It was the default from Nx I guess, would need @mdjastrzebski to confirm. I'm also good with ESM, we should decide wether we roll with it

Yeah, it's the nx default and we just rolled with it

Feel free to tinker with setting up ESM, vitest, etc

grabbou

Thanks for the PR! Being one of the early PRs, you have this opportunity of getting extra comments, not always related to the PR itself!

Anyway, I left some comments regarding the CLI design and architecture. I don't think we discussed this, so I would like to get y'all opinion on the comments.

grabbou · 2024-09-27T14:30:33Z

packages/cli/package.json

+  "bin": {
+    "rnef": "./src/bin.js"
+  },
+  "type": "commonjs",


nit: Isn't this the default?

nx template I guess

grabbou · 2024-09-27T14:31:22Z

packages/cli/package.json

+  "main": "./src/index.js",
+  "typings": "./src/index.d.ts",
+  "private": true
+}


nit: This can leave as a separate TBD, but I think, if we plan to eventually publish each separate module to npm, we should take care of adding all necessary fields to the package.json, such as: description, author, license, repository.

grabbou · 2024-09-27T14:32:09Z

packages/cli/src/lib/__tests__/cli.spec.ts

+
+describe('cli', () => {
+  it('should throw when config not found', async () => {
+    await expect(cli()).rejects.toThrow('rnef.config not found in any parent directory of /');


nit: Do we expect the config to be always defined, a/k/a no defaults? Why?

grabbou · 2024-09-27T14:34:37Z

packages/cli/src/lib/cli.ts

+  .version(version);
+
+export const cli = async () => {
+  const config = await getConfig();


When we last discussed the pom configuration mechanism, you expressed some concerns about non-deterministic nature of arrays. In other words, the result being dependent on the order configs were loaded. How does that concern apply to this?

(I generally think it is hard to solve)

grabbou · 2024-09-27T14:36:50Z

packages/cli/src/lib/cli.ts

+  const config = await getConfig();
+
+  // Register commands from the config
+  config.commands?.forEach((command) => {


nit: One of the design decisions I made in pom was to avoid hardcoding things on the config that do not relate to the configuration of the CLI itself (e.g. plugins, runtime etc.). That said, I always felt like commands were more related to a CLI runtime (in this case, on top of Commander), than to the CLI itself.

In the current design, commands will be loaded ahead of time when you call getConfig() which I guess is fine and somewhat similar to what we've been doing in the CLI, however, the other approach had the benefit of being lazy and more modular, leaving it up to the "commander plugin" itself to define shape of Commands it expects.

I think this is good opportunity to talk about this. I briefly touch-based on it on Slack, but I think we can move that discussion over to here.

grabbou · 2024-09-27T14:46:00Z

packages/cli/src/lib/getConfig.ts

+      ];
+    }
+
+    delete config.plugins;


nit: is there some sort of performance benefit to reusing config and deleting plugins rather than just returning the "final shape" directly from the scratch? I am worried about config having some extraneous properties later, once users start adding new properties on top.

+1 for creating final config as a new object

grabbou · 2024-09-27T14:46:49Z

packages/cli/tsconfig.json

+    "noPropertyAccessFromIndexSignature": true,
+    "noImplicitReturns": true,
+    "noFallthroughCasesInSwitch": true,
+    "resolveJsonModule": true


nit: if we extend the base, shouldn't we put all these into the base already? Some of these don't seem like CLI specific, such as strict: true. I guess we should make sure we have as shared config as possible for consistency.

grabbou · 2024-09-27T14:47:39Z

packages/cli/tsconfig.spec.json

+    "src/**/*.test.ts",
+    "src/**/*.spec.ts",
+    "src/**/*.d.ts"
+, "__tests__/getConfig.spec.ts"  ]


+1 to __tests__ side by side the source, alternatively x.test.ts convention is also nice

grabbou · 2024-09-27T14:48:15Z

packages/cli/tsconfig.spec.json

@@ -0,0 +1,14 @@
+{
+  "extends": "./tsconfig.json",


nit: This is more of a question related to Nx, than this PR. What is the purpose of both lib and spec configs, as well as base?

Sa far as I undestand it:

tsconfig.lib.json is build config

tsconfig.spec.json is for testing

and tsconfig.json is the base one (for the projects, as there is also tsconfig.base.json at the root)

We should discuss whether we want to go with Nx defaults (probably would be easier for us), or to deliberately set the things up in the way want, but to make sure we do it consistently between the projects

grabbou · 2024-09-27T14:49:09Z

tsconfig.base.json

    "module": "esnext",
    "lib": ["es2020", "dom"],
    "skipLibCheck": true,
    "skipDefaultLibCheck": true,


nit: Not related to this PR, but I missed the Nx setup (CC: @mdjastrzebski). I usually prefer using one of @tsconfig by Microsoft that is aligned with the Node version I am using. That way, these do not live hardcoded and are easier to roll as we upgrade the engine requirements.

I've just rolled with Nx defaults, feel free to submit issue / PR with changes.

thymikee · 2024-10-10T13:52:42Z

Superseded by #15

thymikee changed the title ~~Feat/cli config commands~~ feat(cli): create cli package with config, plugins and commands Sep 26, 2024

thymikee requested a review from mdjastrzebski September 26, 2024 14:10

mdjastrzebski reviewed Sep 27, 2024

View reviewed changes

okwasniewski reviewed Sep 27, 2024

View reviewed changes

thymikee mentioned this pull request Sep 27, 2024

test: setup test-helpers package #7

Merged

grabbou reviewed Sep 27, 2024

View reviewed changes

packages/cli/eslint.config.js Show resolved Hide resolved

grabbou reviewed Sep 27, 2024

View reviewed changes

mdjastrzebski mentioned this pull request Sep 27, 2024

FIX: eslint error #8

Closed

thymikee force-pushed the feat/cli-config-commands branch from affa68b to 584c63c Compare September 30, 2024 09:21

thymikee added 8 commits October 5, 2024 18:26

wip: cli package with config loading

700ab69

add basic test with helpers

6b65be3

feat: very basic plugin handling

3cb5aa9

simplify output config, don't pass config to plugins for now

b4b7011

chore: disable faulty lint check

8af3e5e

use test-helpers

a298368

adopt vitest

bfddaf3

wip: move android template to plugin platform

8c067f0

thymikee force-pushed the feat/cli-config-commands branch from 584c63c to 8c067f0 Compare October 5, 2024 21:21

add config package and ios plugin

77cd96a

thymikee closed this Oct 10, 2024

thymikee deleted the feat/cli-config-commands branch October 10, 2024 13:52

LiChuanZhu mentioned this pull request Sep 29, 2025

There is a bug in the gradlew assembleRelease #540

Closed

	action: (config: ConfigType) => void;
	run: (config: ConfigType) => void;


		const extensions = ['.js', '.ts', '.mjs'];

		const importUp = async <T>(dir: string, name: string): Promise<T> => {

feat(cli): create cli package with config, plugins and commands #6

feat(cli): create cli package with config, plugins and commands #6

Uh oh!

Conversation

thymikee commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

mdjastrzebski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okwasniewski Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grabbou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thymikee commented Sep 26, 2024 •

edited

Loading

okwasniewski Sep 27, 2024 •

edited

Loading