Setting query name in cell generator

[Josh-Walker-GM]

Resolves #343

Description
Adds a query argument to the yarn rw g cell command which allows the user to specify a specific GraphQL query name be used in the generated cell.

Todo

• Fix: Prompt conflicts with existing cli interface when asking user to confirm non-unique query name.
• Discuss: Confirm this concerns only the "web" files - changing the implementation of the query name in the "api" is not this generators duty?

[Josh-Walker-GM]

Apologies if opening a few PR's, especially those related to "good-first-issue"'s, in a short period of time is bad etiquette. I'll make this the last one.

[dac09]

Thanks so much for the PRs!

I haven't had a chance to review yet, but just a call out that the query name has to match the imported type too!

[Josh-Walker-GM]

Just glad to hopefully be somewhat helpful and of course absolutely no rush.

I think it was setup somewhat conveniently and it was simply a case of overriding the value of the operationName parameter in the template files. This appears to include the type imports too?

[Josh-Walker-GM]

Okay the current blocker for this PR is the Listr/prompt compatibility.

That is Listr doesn't seem to support user input well at all. An example of this is this current PR which somewhat naively uses prompt within the Listr task and the prompt text is drawn over and it's a big unusable mess.

I think the prompt here is quite useful and in some sense needed to confirm the user accepts an action which might come back to bite them. So, if you want to keep the prompt then you need to ditch Listr but from what I can see the entire set of generators, not just this cell generator, is wrapped up in a Listr based setup.

This leaves me a touch stuck. Maybe there is a way to get prompts and Listr to play nice? Maybe I'd have to rip out this generator from the entire generators setup - yikes!

@Tobbe maybe you have some helpful insight into this conundrum of mine?

[Tobbe]

I don't have an easy/quick solution I'm afraid. You seem to have a good grip on the current state of things. We're really starting to run into the limits of what Listr can handle. As I said over in that PR I love to link 😉 #3989 I think tasktree is worth a look

It would be super valuable if you could do a quick experiment with Lstr2 and tasktree and any other options you can find and see how they would work with our CLI

If we find a tool (or tool combo) that we really believe in I don't see a problem with switching over. It'd have to be a process with a lot of really small steps, switching out for one command at a time

[Josh-Walker-GM]

Okay this PR's original issue is over 2 years old so it's clearly not pressing 😆. I'll produce a quick experiment with Listr2 which has docs on user input support with enquirer. Both seem very popular from my initial look. I'll experiment with tasktree too if I find the time.

[Josh-Walker-GM]

Okay so I had a start at testing with listr2 and it seems like a good approach at least from what I've experience with this cells generator. There doesn't appear to need to be a major overhaul to move from listr to listr2.

NOTE: In this code I've updated lib and other function to use listr2 but only updated the cells generator to use it. It most likely breaks all the other generators or even commands...

Preview of the behaviour I can get with this cells example:

Assuming we were to update to use listr2 the only steps needed to prompt and act on it are something like I've done here:

const answer = await options.listr2task.prompt({
  type: 'confirm',
  name: 'confirmed',
  message: `Specified query name: "${operationName}" is not unique! Do you wish to continue?`,
  initial: false,
})
 if (!answer) {
  options.listr2task.skip('Skipped to prevent non-unique query name.')
  options.listr2ctx.skip = true
}

Then from within the parent task body:

if (_ctx.skip) {
   return
}

This setting and then checking for a skip flag in the context in order to terminate the task seems perhaps a little clunky but there needs to be some way to return from the parent task body and ctx is the reasonable messenger I think.

In principle since the transition from listr to listr2 does not appear major I may be able to create a PR for it. Assuming everyone was on board with it?

I haven't ignored tasktree. I just seen that it would need a larger overhaul and seems to provide only progress bars as a visual addition compared to listr2. Although, I haven't explored it thoroughly.

[Tobbe]

I haven't ignored tasktree. I just seen that it would need a larger overhaul and seems to provide only progress bars as a visual addition compared to listr2. Although, I haven't explored it thoroughly.

What initially got me interested in tasktree wasn't the progress bars. It was its (potential) better control over its output. Especially with Listr you often get a lot of duplicated output. And sometimes if you try to add console logs to the output it doesn't show up at all. Listr2 is said to be better in these regards, but apparently still hadn't fully figured it out. That's where tasktree comes in. You (probably) won't see it unless you have lots of output spanning more than one screen. Read more here listr2/listr2#296

[Josh-Walker-GM]

I had seen this issue mentioned in another redwood PR. Does appear that listr2 can use the same stdout-update that solves this in tasktree however it seems to only be the beta branch of listr2 and it's been this way for over a year now... Maybe we'll need to seek a friendly progress update on that over at listr2 as it does seem to be maintained.

In my head it comes down to three options then:

1. Keep searching for an alternative cli library beyond listr2 or tasktree
2. Strip out listr and use tasktree. This means a relatively bigger change to the codebase and some learning and debugging how tasktree works best and how to get features such as prompting to play nice with it. Big work but could pay off long term.
3. Switch from listr to listr2. This is a smaller change to the codebase. If we did do this we'd have all our current functionality and the addition of nicely working prompts. At the expense of keeping an already existing issue of the underlying listr2 that it hasn't completed it's switch to stdout-update. We'd just have to hope the fix is pushed eventually in the next major update of the package.

I know I'd lean towards option 3 but at the end of the day I'm just a random guy who has never worked on a big js project like this and hadn't even looked into the codebase a week ago 😆

[dac09]

Thanks @Josh-Walker-GM for the effort into this!

I think I'd prefer to disambeguate the two issues - agree that LIstr has limitations, but I'd prefer to switch the libraries in a separate PR - it's a bigger piece of work, as you can see! Even if we do the switch little at a time, I think it's better for us to make sure we don't introduce sideffects like this!

For the functionality you're adding to this PR, I think just erroring out with the message "Query name ${queryName}" is not unique would be just fine I think!

[Tobbe]

I think I'd prefer to disambeguate the two issues - agree that LIstr has limitations, but I'd prefer to switch the libraries in a separate PR

@dac09 This is what you're looking for :) #6444

[dac09]

Requesting a few small changes

[dac09]

Thanks @Josh-Walker-GM 🚀🚀

Added a test on your behalf

[jtoar]

Hey all, saw this change when merging the patch release branch back into main. Could we go without this change, or is this package necessary for this PR? I noticed we didn't change the CRWA template, so something may be missing if this one is necessary

[Tobbe]

Interesting that it shows up here now.
Danny and I discovered a while ago that it's been missing as a web side dependency.
Let me try to find a PR that's relevant

[Tobbe]

Here's the PR
https://github.com/redwoodjs/redwood/pull/6221/files

humanize-string should not be a dep in the test project
Ackshually... Let me check again

So. That PR I linked above removed the import of humanize-string from all the scaffolded components in the test project (and any actual RW app too). Instead it moved to the new /web/src/lib/formatters.ts file.

So it should be added as a dependency to the web side as soon as you scaffold a model.

Previously it wasn't an explicit dependency on the web package. But it was a dependency on the cli package, so we got away with it.