Generators: rollback changes if an error occurs

[cannikin]

Generators: rollback changes if an error occurs

[cannikin]

We do have a --force option that will overwrite files if they exist!

I've actually got a fix for this specific problem (it will just skip scaffold.css if it already exists) waiting in a PR.

Ideally we want a more interactive Do you want to overwrite? (y/n/a) for each file but that's for the future!

[cannikin]

@thedavidprice This was meant to be a feature/enhancement issue but turned into a bug report. haha I think this IS a feature we want at some point: right now if a generator fails for some reason you'll have half the files created and half not.

[cannikin]

Hmmm, good question. I'm not sure what happens if some of the files don't exist when you run the destroy command, but it would be pretty ironic if then that blew up! But in theory it does know how to undo everything that the generate command did...

I can try to test this out at some point but it may be a few days until I get around to it!

[chris-hailstorm]

ENVIRONMENT

$ yarn rw --version
0.0.1-alpha.31

OBSERVATION

Running the Posts generation example from the tutorial, only some files are generated:

$ yarn rw g scaffold post
  ❯ Generating scaffold files...
    ✔ Writing `./api/src/graphql/posts.sdl.js`...
    ✔ Writing `./api/src/services/posts/posts.js`...
    ✔ Writing `./api/src/services/posts/posts.mdx`...
    ✔ Writing `./api/src/services/posts/posts.test.js`...
    ✖ Writing `./web/src/scaffold.css`...
      → /Users/chris/proj/aw-redwood/aw/web/src/scaffold.css already exists.
      Writing `./web/src/layouts/PostsLayout/PostsLayout.js`...
      Writing `./web/src/pages/EditPostPage/EditPostPage.js`...
      Writing `./web/src/pages/PostPage/PostPage.js`...
      Writing `./web/src/pages/PostsPage/PostsPage.js`...
      Writing `./web/src/pages/NewPostPage/NewPostPage.js`...
      Writing `./web/src/components/EditPostCell/EditPostCell.js`...
      Writing `./web/src/components/Post/Post.js`...
      Writing `./web/src/components/PostCell/PostCell.js`...
      Writing `./web/src/components/PostForm/PostForm.js`...
      Writing `./web/src/components/Posts/Posts.js`...
      Writing `./web/src/components/PostsCell/PostsCell.js`...
      Writing `./web/src/components/NewPost/NewPost.js`...
    Adding scaffold routes...
    Adding scaffold asset imports...
✨  Done in 1.20s.

One file already existed from a prior run (=> me not cleaning everything up). The step with "X" (Writing ./web/src/scaffold.css...) failed because the file already existed. The files that were generated before the "X" do exist on disk -- but aren't useful because they depend on files that come later. The files that follow the "X" do not exist on disk, although the CLI output does say "Writing ..." and no per-line error message.

SUGGESTION

Possibly an --overwrite flag to allow clobber of previous generation runs.

Some form of all-or-nothing generation (with awareness of --overwrite) would be best. Hard to diagnose and fix either a partial generation (1st gen) or partial overwrite (2nd/later gen).

[ybakos]

Additional feedback FWIW.

Assuming yarn rw g scaffold foo will be run many times, it will always fail after the first run, due to the existence of scaffold.css, leaving the debris of an incomplete scaffold generation.

[thedavidprice]

@cannikin Ok to close this one out?

[thedavidprice]

@cannikin do you think the Destroy command covers this feature? It's not 1-to-1, but maybe this feature could leverage Destroy -- e.g. if error occurs it would prompt to run cleanup via Destroy.

[jtoar]

An update on this: the presence of scaffold.css doesn't cause the command to blow up anymore. To reproduce this, generate a scaffold and delete most of the files, leaving just one of the ones that's generated later.

[Josh-Walker-GM]

@cannikin Do think there still interest in this? I do think it would be good to rollback on an error by default.

Take #6840 as a recent example of when it would have been much clearer to the user an error occurred if we'd have rolled back and we wouldn't have ended up in a broken state.

If the destroy commands know how to revert the changes of the associated generate command then it might be possible just to add a listr2 rollback task. This addition of the rollback would probably only need to occur when the helper function:

redwood/packages/cli/src/commands/generate/helpers.js

Lines 206 to 222 in 2bc3a54

	const tasks = new Listr(
	[
	{
	title: `Generating ${componentName} files...`,
	task: async () => {
	const f = await filesFn(options)
	return writeFilesTask(f, { overwriteExisting: options.force })
	},
	},
	...includeAdditionalTasks(options),
	],
	{
	rendererOptions: { collapse: false },
	exitOnError: true,
	renderer: options.verbose && 'verbose',
	}
	)

Perhaps it might not be a lot of work to implement this feature, assuming the destroy commands can revert.

[cannikin]

I’d love to do it, but my worry is that the destroy commands haven’t really been kept up to date…I’d be surprised if they all still worked.To really do it the right way I figured the generators really need to be refactored to keep track of what they’re creating so they can automatically be destroyed or rolled back without maintaining separate create/destroy commands like we have now. But, if the destroy commands ARE in sync then you’re right: in case of any error just run the destroy command and we’re good to go!On Nov 20, 2022, at 8:38 AM, Josh GM Walker ***@***.***> wrote: @cannikin Do think there still interest in this? I do think it would be good to rollback on an error by default. Take #6840 as a recent example of when it would have been much clearer to the user an error occurred if we'd have rolled back and we wouldn't have ended up in a broken state. If the destroy commands know how to revert the changes of the associated generate command then it might be possible just to add a listr2 rollback task. This addition of the rollback would probably only need to occur when the helper function: https://github.com/redwoodjs/redwood/blob/2bc3a544167538da5c12e66902f6a385f4a374d4/packages/cli/src/commands/generate/helpers.js#L206-L222 Perhaps it might not be a lot of work to implement this feature, assuming the destroy commands can revert. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[Josh-Walker-GM]

Excellent that's what I was hoping to learn, if these destroy commands were working well and if they were the preferred direction forward. Since they don't seem to be loved I might have a think about how it could be done with as minimal a change to the current generators as possible and without relying on these destroy commands. Thanks!