Add destroy command

[antonmoiseev]

I just started working on this (#80). It currently takes more exploration than actual code writing, but I'd rather open the PR early on to have a chance to ask questions, and hopefully get the feedback while it's still very little code to review.

Several questions that I have so far:

1. Probably the biggest concern currently is reusing files() from the corresponding generate commands. files() reads actual files content from disk and renders the templates, which is completely unnecessary for destroy command. Should we consider refactoring this and splitting file paths resolving and template rendering into separate functions? It could be done as another issue after the initial version of destroy is done.

2. Do you have a preference which word to use for console messages: "destroy", "delete" or "remove" (expand to see examples)?

   • Destroying component files...
   • Removing component files...
   • Deleting component files...

3. After deleting generated files the directory might or might not be empty (users can create files manually). Do you think we should get rid of empty directories as well?

4. I find it ambiguous to refer to different types of entities that CLI can generate/destroy as "components":

   

   Have you discussed this previously? Did you decide to settle on this term for now?

[antonmoiseev]

Just ran into another issue with reusing files() from generate commands. Since files() renders the templates templateVars that often come from generate builder flags must be defined. To avoid defining fake builder flags for destroy commands I came up with a workaround - wrapping files() into another function, so we can pass missing templateVars that the templates expect. See an example here.

[jonniebigodes]

@antonmoiseev wow this is a good one. If you allow me my two cents here. I think it would be best to use "remove" instead of destroy. Wording wise it would be a better approach

[cannikin]

@antonmoiseev Thanks so much for doing this PR!

1. Agree, we should refactor the generators to have a function that just returns the list of files and not their contents. This will help with the Generators: rollback changes if an error occurs #82 feature as well. But yeah that can probably wait until after this is merged and working.

2. I've been modeling the generators from those in Rails and they use "destroy" so that's what my brain defaulted to, but I'm open to another synonym if people like another one better. Can I throw "ungenerate" into the ring? 😀

3. I think we can delete an empty directory, yeah.

4. Interesting, yeah "component component" isn't right, it should just be "component". We do want to append it to the other things though, like "layout component", "page component", etc. The quick way would be to override the desc property that comes out of the createYargsForComponentGeneration call in the component generator.

[mojombo]

I think "destroy" is a good name, as it's quite dangerous sounding, which reflects the truth of the command. You should think twice before you call it.

[antonmoiseev]

@cannikin thank you for the feedback! Sorry for "ungenerate" though 😬, left "destroy" for now as Tom suggested.

Just to give you an update on the progress - all commands seem to work fine now, but I need to get rid of empty dirs and add tests. One thing I'm not sure about is scaffold.css file. When destroying a scaffold there is no easy way to identify whether anyone else is still using scaffold.css file, so I cannot decide whether it needs to be cleaned up or not. I was thinking about searching for rw-scaffold CSS class within layout components, but probably it would be too much. Do you have any suggestions here?

[cannikin]

I think it’s okay to leave it. Checking if anything else uses it seems like more trouble than it’s worth.

…

On May 1, 2020, at 10:48 PM, Anton Moiseev ***@***.***> wrote:  @cannikin thank you for the feedback! Sorry for "ungenerate" though 😬, left "destroy" for now as Tom suggested. Just to give you an update on the progress - all commands seem to work fine now, but I need to get rid of empty dirs and add tests. One thing I'm not sure about is scaffold.css file. When destroying a scaffold there is no easy way to identify whether anyone else is still using scaffold.css file, so I cannot decide whether it needs to be cleaned up as well. I was thinking about searching for rw-scaffold CSS class within layout components, but probably it would be too much. Do you have any suggestions here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[antonmoiseev]

Well, I now clean up empty dirs after destroy runs but feel free to let me know if you find it too verbose. Here is probably an extreme version of destroy log:

[cannikin]

That log looks fine to me, I like seeing everything it's doing!

[mojombo]

Agreed, you definitely want a record of everything being deleted.

[thedavidprice]

Hi @antonmoiseev is this one ready for review + merge? Please disable "draft" and loop in Rob Cameron if so. Thanks!

[antonmoiseev]

Feature-wise it's ready for review, but tests are missing. Destroy commands primarily consist of two parts:

1. Figuring out which files need to be deleted / reverted.
2. Deleting and reverting files.

Since destroy reuses files() from corresponding generate commands the first part is already covered by generate tests. To test the second part I need to run tests against actual file system, but it looks like you do not currently do this. So, the only idea that I came up with is to test removeRoutesFromRouterTask().

@cannikin could you please review this PR? If you have any tests suggestions, please let me know, I'll address them.

[cannikin]

Hmmm... @peterp can you think of any way to test that files are being deleted? We discussed mocking out the filesystem at one point, but when we went to just testing the result of files() rather than the generator itself (which would actually write files) I think we abandoned that approach.

In Ruby there's a gem called mocha that lets you set expectations on method calls. So you can say "unlinkSync should be called one time, and with the following argument(s)". It will then "absorb" that method call and not really do it, so you don't get any errors if the files don't actually exist. Is there similar functionality in Jest? The nomenclature for testing is all over the map, but in mocha these are called "mocks". There are also "stubs" which allow you to redefine what a method returns.

[thedavidprice]

@RobertBroersma any test-related suggestions here?

[RobertBroersma]

@thedavidprice I'm not familiar with the cli package so much, but in general I like to keep my tests as close to the user as possible. So for commands I would for every use-case:

1. Arrange Code/Files/Whatever the command deals with.
2. Execute the command (possibly through exec but probably just call the function behind yarg)
3. Assert the Code/Files/Whatever the command deals with is now correct.

@cannikin There certainly is something like that in Jest! There's spies (for "spying" on functions, but leaving them intact), and mocks, for faking out functions.

The Jest docs on mocks actually feature an example for mocking fs, which would be the only thing I would indeed mock: https://jestjs.io/docs/en/manual-mocks

Hope that helps!

[antonmoiseev]

There is mock-fs package that can mock entire fs module. But I believe you mentioned in the community forum that you prefer to mock as little as possible. In Dart ecosystem there is test_descriptor package that allows defining and verifying directory structures. It creates directories in the system's temp folder and teardowns everything after the test run. I was hoping to find something similar in node.js as well.

[RobertBroersma]

I don't have experience with testing stuff on the file system, but if it's workable without mocking it that would be cool! I would be surprised if there are no packages available for that!

If not maybe we could make something simple with fs for assertions and tools like rimraf for cleanup :)

[cannikin]

I have no idea what to do about testing these methods that are all wrapped up in Listr calls, or the actual generators themselves. For now we took the easy path and just tested that files() returned from each generator contains the proper list of file paths and contents—we sort of cross our fingers and hope that everything else around the generators does their jobs. 😬Unfortunately none of us are JS testing experts and know of a good/recommended/easy way to write those tests.

As @RobertBroersma said:

There's spies (for "spying" on functions, but leaving them intact), and mocks, for faking out functions.

If I was writing this in Ruby I would do exactly this—mock out the file system calls (like fs.unlinkSync()) and verify that the call was made with the proper argument (the path of the file to be deleted):

# The mocha library automatically hooks into all classes so you 
# can `expects` on any class (like the file library `File`)

test "sdl destroyer deletes the sdl file" do
  destroyer = SdlDestroyer.new
  File.expects("delete").with("/path/to/project/api/src/graphql/post.sdl.js")

  destroyer.handler(:post) 
end

No need to create fake files and have the OS actually delete them—we can be reasonably sure that will work as expected. We just want to make sure our code is making the proper calls to those low level functions. Maybe you can mock out that file system call and then invoke the destroyer's handler() function, and everything will magically work?

[RobertBroersma]

@cannikin @antonmoiseev Another Jest feature that could be useful for template testing is snapshots! https://jestjs.io/docs/en/snapshot-testing

Instead of writing the file to the file system you keep a snapshot of the correct output.

Jest Snapshots aren't often the right choice but I think this is a perfect fit!

This doesn't really make sense for deleting files I guess. For that I agree it's probably easier to mock out the filesystem.

[thedavidprice]

I have no idea what to do about testing these methods that are all wrapped up in Listr calls, or the actual generators themselves.

Agreed this is messy. And I'm super guilty of this re: yarn-redwood-create.js 😬The Listr implementation is a leading reason there's a lack of correlating tests. And I've been thinking it might be best practice for us to create functions outside Listr, which would then be called as needed per task. (Am I correct that something like this would move us a step closer to testing @cannikin?)

But this conversation made me curious about how/if you can isolate and run specific tasks within the Listr array. And, indeed, you can. The following runs the first task without any of the Listr elements (e.g. no display title, etc):

const tasks = new Listr([ ... ])

tasks._tasks[0].run()

Does isolating each task like this help with testing?

FYI --> Here’s_ the Listr structure for an array of three tasks with titles "First", "Second", "Third":

Click here to view object

[
  Task {
    _isScalar: false,
    observers: [],
    closed: false,
    isStopped: false,
    hasError: false,
    thrownError: null,
    _listr: Listr {
      _options: [Object],
      _tasks: [Circular],
      concurrency: 1,
      _RendererClass: [Function: UpdateRenderer],
      exitOnError: undefined
    },
    _options: {
      showSubtasks: true,
      concurrent: false,
      renderer: 'default',
      nonTTYRenderer: 'verbose'
    },
    _subtasks: [],
    _enabledFn: undefined,
    _isEnabled: true,
    output: undefined,
    title: 'First',
    skip: [Function: defaultSkipFn],
    task: [Function: task]
  },
  Task {
    _isScalar: false,
    observers: [],
    closed: false,
    isStopped: false,
    hasError: false,
    thrownError: null,
    _listr: Listr {
      _options: [Object],
      _tasks: [Circular],
      concurrency: 1,
      _RendererClass: [Function: UpdateRenderer],
      exitOnError: undefined
    },
    _options: {
      showSubtasks: true,
      concurrent: false,
      renderer: 'default',
      nonTTYRenderer: 'verbose'
    },
    _subtasks: [],
    _enabledFn: undefined,
    _isEnabled: true,
    output: undefined,
    title: 'Second',
    skip: [Function: defaultSkipFn],
    task: [Function: task]
  },
  Task {
    _isScalar: false,
    observers: [],
    closed: false,
    isStopped: false,
    hasError: false,
    thrownError: null,
    _listr: Listr {
      _options: [Object],
      _tasks: [Circular],
      concurrency: 1,
      _RendererClass: [Function: UpdateRenderer],
      exitOnError: undefined
    },
    _options: {
      showSubtasks: true,
      concurrent: false,
      renderer: 'default',
      nonTTYRenderer: 'verbose'
    },
    _subtasks: [],
    _enabledFn: undefined,
    _isEnabled: true,
    output: undefined,
    title: 'Third',
    skip: [Function: defaultSkipFn],
    task: [Function: task]
  }
]

[cannikin]

It seems like being able to test the individual Listr commands would help...for the destroy one you'd still need the files to exist for them to actually successfully delete, though. I feel like just mocking out fs.unlinkSync is the simplest thing we can do in this case. For the task that edits the Routes file we'd need to have a real one somewhere and then compare the before and after to see if they were edited correctly.

But yeah testing the Listr tasks seem like it would help for all the other stuff that aren't covered by any tests currently. We could go up a mocking level and just test that the Listr tasks call the correct methods in the generators, rather than testing the whole generator stack again. But too much of that and it starts getting pretty brittle...change anything anywhere and you've got this huge cascade of mocks to fix. Ugh.

@antonmoiseev how do you feel about mocking out the file system calls as described here and just checking that unlinkSync is called with the proper paths? I'm not sure how actually invoke the generator though...can you just invoke handler() from, for example, commands/destroy/page/page.js? I wonder if the fact that it's async causes any additional issues...

[antonmoiseev]

how do you feel about mocking out the file system calls as described here and just checking that unlinkSync is called with the proper paths?

@cannikin Sounds good, that's what I tried, but was mocking readFile() and writeFile() from lib/index.js. Not sure though why you would want to invoke generators? Since we mock file system calls we don't need files to exist, we can just pass the name and verify that destroy attempts to delete the right files. I'll add the tests over this weekend.

[jtoar]

@thedavidprice Totally; just put up #559. Just want to make sure I got the help message right:

Output for yarn rw g --help

~/redwood-app$ yarn rw g --help
yarn run v1.22.4
$ /redwood-app/node_modules/.bin/rw g --help
rw generate <type>

Save time by generating boilerplate code.

Commands:
  rw g auth <provider>     Generates auth configuration.
  rw g cell <name>         Generate a cell component.
  rw g component <name>    Generate a component component.
  rw g function <name>     Generate a function
  rw g layout <name>       Generate a layout component.
  rw g page <name> [path]  Generates a page component.
  rw g scaffold <model>    Generate Pages, SDL, and Services files based on a
                           given DB schema Model.
  rw g sdl <model>         Generate a GraphQL schema and service object.
  rw g service <name>      Generate a service component.

Options:
  --help     Show help                                                 [boolean]
  --version  Show version number                                       [boolean]

Output for yarn rw g scaffold --help

~/redwood-app$ yarn rw g scaffold --help
yarn run v1.22.4
$ /redwood-app/node_modules/.bin/rw g scaffold --help
rw g scaffold <model>

Generate Pages, SDL, and Services files based on a given DB schema Model.

Positionals:
  model  Model to scaffold. Optionally may use "path/model" to nest files by
         type at the given path directory (or directories)            [required]

Options:
  --help     Show help                                                 [boolean]
  --version  Show version number                                       [boolean]
  --force                                             [boolean] [default: false]

[antonmoiseev]

@jtoar Sorry for raising this question a bit too late after the work was already done. I was going to offer my help, but you were too quick! Thank you!

Is the "it" you're referring to the Destroy command? If so, how would supporting --path differ from your current implementation.

@thedavidprice I was referring specifically to destroy scaffold command, but since we are dropping --path, I have no more questions. Thank you.

[jtoar]

@antonmoiseev No worries! It was a super simple fix so thank you for bringing it up--we can get it right before getting it out.

[thedavidprice]

@antonmoiseev Is there further work in progress here? If not, we are looking to release v0.7 tomorrow and it would be great to include this. But all good if need more time.

Thanks!

[antonmoiseev]

@thedavidprice Yep, I saw your RC release, so full steam ahead, aiming to push final tests in a couple of hours 😄. Hopefully it will make it into v0.7, but no worries if it won't be good enough or fully reviewed by tomorrow.

[antonmoiseev]

@cannikin It's ready for review, however as soon as #559 is merged, I'll need to adjust destroy scaffold command since the parameter names will change.

/cc @thedavidprice

[thedavidprice]

#559 has been merged

[antonmoiseev]

Adjusted to scaffold generator changes, merged with master, ready for review now.

[peterp]

Thanks for this! It looks good to me, but I'm going to check it out and test locally.

[antonmoiseev]

@peterp Thank you!