Architecture, Config, CLI, API — Recommendations & Discussion

[waynevanson]

I've been thinking about and working on this project for weeks and am growing a passion towards it.
Thank you for all being so accepting, patient and prompt in your responses.

I'd like to voice some ideas and possible solutions. There's a lot here, so take your time to read and respond if you choose to do so.

Quality of life

Refactor

A great idea, but some of the code is messy, hard to interpret and difficult to test. I've posted a few pulls to refactor code. I will keep doing this, taking anything from this discussion into account.

Config File

I personally would prefer a configuration that supports destiny.config.(ts|js), with the default export as what will be read.

If typescript, the user should have ts-node installed . This way we can assign an interface to the object. I'm pretty sure webpack and/or rollup have this feature built in.

Architecture

I think we can start researching how we may implement an architecture that can support other file extensions via middleware. Prettier has something similar for each language they support, which include parsers and formatting rules.

The way this program could be laid out:

1. Create graph representing current structure
   a. Find a file
   b. Add import path to graph
2. Create graph representing new structure
   a. ...details
3. Move files and folder structure

There are some details that I've overlooked, but it's general.

Graphs

Currently we're managing our own graphs. It is simple with object-index pattern:
{ [index: string]: string }.

It works with strings as the nodes, but what if we ever needed to use an instance on each end? We'd have to rewrite it.

I believe we should go down the OOP route for our graph generation and management. It would make extensibility and management easier. I love functional programming, but well managed mutability is where the power is in graphs!

We could use a library for manipulating graphs called cytoscape.js, which comes with some fantastic abstractions.

On first glance it looks like it's built to support the DOM. However, it can run in headless mode, meaning all the rendering capabilities are disabled and it can be used to strictly manipulate data. Score!

CLI API

I wrote a cli in yargs and came up with this:

Usage: index.ts [options] [files / regex]

Dry run and preview directory structure:
  index.ts src/**/*.*

Write the changes:
  index.ts -w src/**/*.*


Options:
  --help             Show help                                         [boolean]
  --version          Show version number                               [boolean]
  --config, -c       path to your config file.
                     Can be in any language supported by destiny.
  --dir, -d          Sets the root directory
  --init             Creates a `destiny.config.*`, defaulting to `.ts`
                     file extension                        [choices: "ts", "js"]
  --interactive, -I  Prompts user to process directory structure
                     when a dry run is complete
  --ignore, -i       Locations to be ignored whilst processing           [array]
  --subfolder, -s    Operates on each folder in the directory,
                     instead of on the directory itself                  [count]
  --test, -t         A regex to match test files against                [string]
  --write, -w        Writes the changes to the directory               [boolean]

Supported formats:
  .js, .jsx, .ts, .tsx

Generated by this:

import * as yargs from "yargs";

const argv = yargs
  .usage(
    `
Usage: $0 [options] [files / regex]

Dry run and preview directory structure:
  $0 src/**/*.*

Write the changes:
  $0 -w src/**/*.*
  `
  )
  .epilogue("Supported formats:")
  .epilogue("  .js, .jsx, .ts, .tsx")
  .demandCommand(1)
  .option("config", {
    alias: "c",
    description: `path to your config file.
    Can be in any language supported by destiny.`
  })
  .option("dir", {
    alias: "d",
    description: `Sets the root directory`
  })
  .option("init", {
    description: `Creates a \`destiny.config.*\`, defaulting to \`.ts\`
    file extension`,
    choices: ["ts", "js"]
  })
  .option("interactive", {
    alias: "I",
    description: `Prompts user to process directory structure
    when a dry run is complete`
  })
  .option("ignore", {
    alias: "i",
    array: true,
    description: `Locations to be ignored whilst processing`
  })
  .option("subfolder", {
    alias: "s",
    count: true,
    description: `Operates on each folder in the directory,
    instead of on the directory itself`
  })
  .option("test", {
    alias: "t",
    string: true,
    boolean: true,
    description: `A regex to match test files against`
  })
  .option("write", {
    alias: "w",
    boolean: true,
    description: `Writes the changes to the directory`
  }).argv;

[sQVe]

Refactor

A great idea, but some of the code is messy, hard to interpret and difficult to test. I've posted a few pulls to refactor code. I will keep doing this, taking anything from this discussion into account.

I disagree. I think the code base is in better shape now and that we should try to move our focus onto features and bug fixes. Code improvements can be introduced combined into those features and fixes.

Config File

I personally would prefer a configuration that supports destiny.config.(ts|js), with the default export as what will be read.

This is already supported via https://github.com/davidtheclark/cosmiconfig.

If typescript, the user should have ts-node installed . This way we can assign an interface to the object. I'm pretty sure webpack and/or rollup have this feature built in.

I do not understand what you mean by this. The cli currently ensures type correctness.

Architecture

I think we can start researching how we may implement an architecture that can support other file extensions via middleware. Prettier has something similar for each language they support, which include parsers and formatting rules.

I disagree. I think we should flesh out and finish support for the basic languages we already have. There are plenty of critical bugs here and there. Once we have a working solution we can start looking at branching it out into other languages.

Graphs

Currently we're managing our own graphs. It is simple with object-index pattern:
{ [index: string]: string }.

It works with strings as the nodes, but what if we ever needed to use an instance on each end? We'd have to rewrite it.

In what cases do we need instances as our graph values? The graph is currently {[key: string]: string[]} which should be fine for file paths. Could you give an example?

CLI API

I wrote a cli in yargs and came up with this:

I dislike yargs as it makes something abstract which does not need to be abstract. Furthermore it is hard to properly integration test a yargs based application. I'd prefer to stay on path with how our current cli is implemented. With that said, I'm open for changes but only if it is testable and adds important features.

[AnatoleLucet]

I also think the cli is better to stay "handily made". That aside I have planned to do a function to generate the help message. Manually adding spaces for text alignments is messy in my opinion.

[waynevanson]

This is already supported via https://github.com/davidtheclark/cosmiconfig.

I do not understand what you mean by this. The cli currently ensures type correctness.

I see, apologies for not seeing this earlier. Great news!

we should try to move our focus onto features and bug fixes. Code improvements can be introduced combined into those features and fixes.

I think we should flesh out and finish support for the basic languages we already have. There are plenty of critical bugs here and there

Perhaps I'll add some time into test coverage #80 instead :)

The graph is currently { [key: string]: string[] } which should be fine for file paths. Could you give an example?

It's fine for just file paths, but will there ever be a case where we may need to add more information than just a file path? I think we would if expanding to other file extensions.

A use case could be interacting with destiny in a callback function. I'd want a callback with all the details about a node, with access to navigate the graph via the abstraction.

FileNode {
  path: "/home/user/Documents/project/src/index.ts",
  isProcessed: true,
  testFiles: [ ... ],
  // absolute locations. These would be other "FileNode" instances.
  nodes: [
    "/home/user/Documents/project/src/index/dependency-1.ts",
    "/home/user/Documents/project/src/index/dependency-2.ts",
    ],
  // relative import paths. Only one needed because they're the same.
  edges: [ ... ]
}

I'm sure there's more applicable uses.

I dislike yargs as it makes something abstract which does not need to be abstract

I don't think it's perfect by design and can be misused in terms of maintainability: neat and declarative.

My biggest reason for using yargs is that it's types safe. All options are typed so if they're put into a switch statement, TS can advise when a case is missing.

I'm not fussed, but definitely something I'll be using in my personal projects from now on.

Furthermore it is hard to properly integration test a yargs based application

Why would you say that it is difficult? I don't believe it would be hard to test against. Wouldn't it be just like any other CLI?

With that said, I'm open for changes but only if it is testable and adds important features.

I like the option API used and the array argument type in this case. --ignore-files could be src/precious/**/** or could be src/precious/file-1.ts/ src/precious/file-2.ts. I'm certain this could be written ourselves, but it's already done and easy to read.

[AnatoleLucet]

I like the option API used and the array argument type in this case. --ignore-files could be src/precious// or could be src/precious/file-1.ts/ src/precious/file-2.ts. I'm certain this could be written ourselves, but it's already done and easy to read.

I've already tried something about a week ago, but never found time to continue working on it.
Here's how I've made (updated to today's develop branch) the flag's value implementation (file):

const parseArgs = (args: string[]) => {
  let isValue = false;

  args.reduce<Partial<Config>>((acc, arg, i) => {
    if (isValue) {
      isValue = false;
      return acc;
    }

    switch (arg) {
      case "-h":
      case "--help":
        acc.help = true;
        break;
      case "-V":
      case "--version":
        acc.version = true;
        break;
      case "-w":
      case "--write":
        acc.write = true;
        break;
      case "--flag-with-value":
        acc.flagWithValue = args[i + 1];
        isValue = true;
        break;
      default: {
        if (fs.existsSync(arg) || glob.hasMagic(arg)) {
          acc.include = [...(acc.include ?? []), arg];
        }
      }
    }

    return acc;
  }, {});
};

So you can do:

destiny src --flag-with-value "a value" --another-flag

And "a value" will be stored in mergedConfig.flagWithValue.

For a list something like this is possible:

destiny src --flag-with-value aFile,anotherFile,stillAFile --another-flag

Then in the case: you can just args[i + 1].split(",")...

Maybe there's a better way to do this, but that's the way I did it.

---

FileNode {
 path: "/home/user/Documents/project/src/index.ts",
 isProcessed: true,
 testFiles: [ ... ],
 // absolute locations. These would be other "FileNode" instances.
 nodes: [
   "/home/user/Documents/project/src/index/dependency-1.ts",
   "/home/user/Documents/project/src/index/dependency-2.ts",
   ],
 // relative import paths. Only one needed because they're the same.
 edges: [ ... ]
}

In my opinion it would be great to have something like this. We could for example store the file's extension, the file's parent, etc... in it so we don't have to manually search (again) for these everywhere in the engine. Because currently there's a lot of repetition about this.

[sQVe]

It's fine for just file paths, but will there ever be a case where we may need to add more information than just a file path? I think we would if expanding to other file extensions.

It's more than enough to use what we already have. If we in the future see a need to expand this we'll do that but we should not try to fix something that is a maybe. We need to keep it as simple as possible for as long as possible.

I don't think it's perfect by design and can be misused in terms of maintainability: neat and declarative.

I think yargs is far less readable. Our cli consists of one run function which in turn parses all options and paths by iterating over all arguments with a switch case. All other functionality needed, like arguments for options are easily implemented.

My biggest reason for using yargs is that it's types safe. All options are typed so if they're put into a switch statement, TS can advise when a case is missing.

What do you mean by it being typesafe? It's not more typesafe than the current cli implementation.

Why would you say that it is difficult? I don't believe it would be hard to test against. Wouldn't it be just like any other CLI?

It's hard because it abstracts and binds things together. The current cli can be unit tested easily because all moving parts are isolated and in our control. A yargs application mushes everything and makes a mess.

Try to unit test the cli you wrote and try for yourself.

[sQVe]

I've already tried something about a week ago, but never found time to continue working on it.
Here's how I've made (updated to today's develop branch) the flag's value implementation (file):

This should be implemented with a while loop which would give us the best flexibility. That would allow us to parse multiple values for a specific option etc.

[sQVe]

@AnatoleLucet Something like this:

const parseArgs = (args: string[]) => {
  const parsedArgs: Partial<Config> = {};

  while (args.length > 0) {
    const arg = args.shift();

    if (arg == null) break;

    switch (arg) {
      case "-h":
      case "--help":
        parsedArgs.help = true;
        break;
      case "-V":
      case "--version":
        parsedArgs.version = true;
        break;
      case "-w":
      case "--write":
        parsedArgs.write = true;
        break;
      case "-G":
      case "--gimme": {
        const nextOptionIdx = args.findIndex(x => x.startsWith("-"));

        parsedArgs.gimme = args.splice(0, nextOptionIdx);
        break;
      }
      default: {
        if (fs.existsSync(arg) || glob.hasMagic(arg)) {
          parsedArgs.include = [...(parsedArgs.include ?? []), arg];
        }
      }
    }
  }

  return parsedArgs;
};

[AnatoleLucet]

@sQVe Yep that's better 👍

[waynevanson]

I think this discussion has served its purpose. Closing off.