Add option to setup TailwindUI with TailwindCSS

[olance]

Added a --ui option to the setup tailwind command to automatically setup/configure TailwindUI along with TailwindCSS.

👉 It installs @tailwindcss/ui and requires it in the tailwind.config.js configuration file

(still missing tests)

[olance]

Unless I missed them, it seems there are actually no tests for this part of the code?

Should we add some, or is it assumed that the rw commands aren't tested?

[thedavidprice]

Nice! This option makes a ton of sense.

We do need a little coordination here. I've been working with the author of #1407 for several weeks (see issue #1301), although the PR just came through with yours. He spent a lot of time getting the --force option to work correctly, which is going to conflict with this file. My preference is to get #1407 merged first. Open to suggestions, but how about this for a plan:

1. I loop you in on Fix tailwindcss setup #1407 for review and feedback. Functionality is working great as is. He's open to code changes if needed (especially if they'll be helpful for this).
2. I merge Fix tailwindcss setup #1407
3. There will be merge conflicts to handle here (sorry 😬)
   • Please do add a simple test — we've been letting CLI commands sneak through, but it's just not good practice. Especialy when it comes to being able to add and mod files, as well as overwrite.
4. We'll get this one merged

OK? Too much? Thoughts either way?

[olance]

@thedavidprice sounds perfect!
Shouldn't be much hassle to solve conflicts/reimplement.

I've had a first look at the other PR and it seems it's going to fix what I was pulling my hair off on yesterday 😅

I thought I had broken something that made yarn tailwindcss init fail ("command tailwind css not found") when using --force, without touching that part of the code.
From what I understood, the --check-files step should deal with that!?

[thedavidprice]

From what I understood, the --check-files step should deal with that!?

Yeah, this one is a double gotcha 💥

1. Anytime you're using yarn redwoodtools ... for local development, and you run yarn install in your project, it seems the linking (via copy/paste) process breaks.
2. If the tailwindcss package is already installed, for some SuperStrange™ reason, the yarn binary yarn tailwind would break. So the yarn --check-files was added to remedy this. It does, however, add a lot of time to the process ¯_(ツ)_/¯

[olance]

Ah! Both things I had issues with then! ^_^

[olance]

All right, the branch is rebased!
I'll look into adding tests later today :)

[olance]

Actually @thedavidprice what do you think the testing strategy should be?

I don't have a lot of practice testing that kind of codebase... I'm reluctant to mock execa to check for calls to yarn with the correct packages (as that implies letting implementation details leak into the tests), but not doing so would result in actually calling yarn and installing packages.

That might be a bit too much (and slow!) for tests? ^^

[thedavidprice]

Agreed with those points — this is an ongoing theme regarding the CLI and unit tests. And I don't think it makes sense to test yarn install or the tailwindcss CLI.

I just looked at the test for Auth generator as an example. Turns out there's not much going on, but it could be a template to start with:
https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/generate/auth/__tests__/auth.test.js

One thing we've done in the past is to isolate specific tasks in Listr for tests. See my comment here and the resulting tests for the Destroy command:
#487 (comment)

What if we try to test:

• fails correctly (Auth test for reference)
• can write a file correctly (Listr task default)
• can code mod correctly (Listr task default)
• can overwrite something correctly (Listr task with option)

Where these tests are the most helpful is cross-platform, e.g. in the case of Window.

Open to any/all suggestions. Especially if you feel the scope is too large and or not viable.

[olance]

OK so, if I understand correctly:

• fs functions are being mocked as well as the "generated" files contents
• adequate fs functions (unlinkSync in destroy command case) are spied on
• all Listr tasks are run at once
• assertion are made on appropriate fs functions being called with correct filenames/contents/...

I guess I can reproduce this and apply it to execa as well to assert for yarn calls being made.

Here's the test plan I had written down last week.
I think it covers yours:

→ On pristine app

• Correctly calls yarn add for modules postcss-loader, tailwindcss, autoprefixer and optionally @tailwindcss/ui
• Correctly calls yarn install --check-files 👈 the one caveat to that in my opinion, is that we're assuming this is the correct solution to the binary disappearing. Letting the command run is too costly, but would allow us to check for sure that things look OK.
• Creates postcss.config.js with correct content
• Creates web/tailwind.config.js with correct content, with "future flags" set to true and optionally TailwindCSS UI plugin added
• Adds CSS imports for TailwindCSS

→ On already setup app, without --force
I'd simulate an "already setup app" by just having the files postcss.config.js and tailwind.config.js exist before tests run

• Correctly fails on postcss.config.js being present
• Without an existing postcss.config.js, correctly fails on web/tailwind.config.js being present

→ On already setup app, with --force

• Same checks as pristine app, just a different initial setup

What do you think?

As to "mocking" an app structure that the tasks could work on, do you think I should use fixtures (I'd say so), or some heavy use of fs.__setMockFiles ?

I'll get started and adjust depending on your feedback ^^

[olance]

@thedavidprice as a first step, I have reorganized the code to support the test plan and move in the direction you were hinting at in your comment for the destroy command tests.

I don't feel like running the complete tasks set at once for each test, I don't think it makes a lot of sense.
Because of that I started by extracting the const tasks from the handler and exporting it from the module so that tests could run each tasks independently. But:

1. I'm not a huge fan of exporting stuff outside of modules simply for tests' sake; and in this case there's no reason why tasks should be exported out of the tailwind module
2. I didn't like the idea of exporting the whole function-that-returns-a-Listr-instance and having to have knowledge of the tasks order in my tests to run the appropriate one for each test

So I went with this:

• Created a setup/tailwind/tasks directory
• Extracted each task function into an adequately-named module into tasks, e.g. tasks/installPackages.js
• Imported those functions to "compose" the Listr tasks list and use in the handler
• Stopped exporting tasks out of the tailwind module

I'd say it has the merit to increase testability and improve code legibility by keeping files small and scoped to a single thing at once. It should also make tests slightly clearer as I'll be able to call just the task function I'd like to test, instead of the all set of tasks over and over again like in the destroy service tests.
And finally, it allows us to have both unit tests for tasks functions and some form of "integration test" if we want to, by running the full handler as a blackbox and testing its outcome.

I have pushed the changes to the branch, so you can have a look.
I'm curious to know what you think about all this!

[dthyresson]

@olance given @thedavidprice is away for a few days -- and I have been using (and have purchased Tailwind UI), do you need me to review or test this so we can push the PR forward in his absence?

If, so, let me know what you need.

[olance]

Hey @dthyresson!

Thanks, you could start reviewing but it's not fully ready yet.

It took me some time to understand the testing architecture & helpers and to reorganize the code to make it testable, but now it's just a matter of writing the last few tests ^^

As I'm working on it burst by burst, I might need a few more days before completing everything.

I can request you as reviewer once it's done if you'd rather review the full thing at once!

[dthyresson]

I can request you as reviewer once it's done if you'd rather review the full thing at once!

👍

[olance]

Regarding the stalling tests, I have an intuition this does not come from my own tests, but others from commands/generate/scaffold.

I've been able to run the Github action locally with nektos/act and got the same issue as what's happening here.
But what I can see as well more clearly, is a bucketload of errors related to async callbacks not being called in time:

| @redwoodjs/cli:     Timeout - Async callback was not invoked within the 15000 ms timeout specified by jest.setTimeout.Error: Timeout - Async callback was not invoked within the 15000 ms timeout specified by jest.setTimeout.

There are tens of them, on two tests for the scaffold generator:

@Tobbe @dthyresson dunno if you're familiar with those parts of the code?

I'll try to run my tests—and only them—within my local Github Actions sim and see how things go; then continue investigating depending on the results 🙂

[Tobbe]

Ohh, I remember having timeout issues when I was messing around with the generators. I think I even PRed an increase in the timeout.

Indeed! #845

I never dove deeper into it than that. What happens if you increase it to 20? If so, it might be worth it to see if we can understand exactly why that's needed. Are there new tests added that take longer to run? Or is it just that the total amount of tests have increased? If so, why does that matter? Or is it just that different hardware are slower/faster executing the tests?

[olance]

I never dove deeper into it than that. What happens if you increase it to 20? If so, it might be worth it to see if we can understand exactly why that's needed. Are there new tests added that take longer to run? Or is it just that the total amount of tests have increased? If so, why does that matter? Or is it just that different hardware are slower/faster executing the tests?

So many questions 😅
Thanks for the pointer! I'll see if I can get to the bottom of this :)

[hastebrot]

@olance Yes, this makes sense to me to have four options: v1 without ui, v1 with ui, v2 without ui, and v2 with ui. Those who bought TailwindUI before Tailwind v2 was released still have access to the old TailwindUI code where Tailwind v1 was used.

[olance]

Hey there! I'm back 😅

Work has been crazy for weeks, I can finally dive into these failing tests again.

I think I've got a fix for the stalled tests, but it might need the scrutiny of @thedavidprice or someone else from the core team maybe?

Here goes:

• jest.config.js is currently configured to generally run tests from all *.test.js/ts files, but from all files with a js/ts extension within __tests__ folders: testMatch: ['**/__tests__/**/*.[jt]s?(x)', '**/*.test.[jt]s?(x)'],
• In my tests, I have created a __tests__/execa.mock.js file to abstract away some complexities from my actual tests in __tests__/iniTailwind.test.js. With the above config, that module ends up being run as a test file.
• Line 14 of that execa.mock.js file, I'm calling getPaths() from the Redwood internal package, which looks for the redwood.toml project file. It doesn't find it, because nothing's setup correctly at that stage since this is not an actual test file.
• For some reason, the getPaths function that proxies from the CLI package to the internal package silently absorbs errors from the internal package:

  try {
    return getRedwoodPaths()
  } catch (e) {
    console.error(c.error(e.message))
    process.exit(0)  // <--- exit without error
  }
}

So when running the test file that's not a test file, the search for a redwood.toml ends up in an error that's not being reported as such because of the catch + exit(0). As a result, Jest considers everything went well but things seem to be left in a weird state and the tests stall.

To fix this, I have thus corrected the packages/cli/jest.config.js match pattern to only run actual test files:

- testMatch: ['**/__tests__/**/*.[jt]s?(x)', '**/*.test.[jt]s?(x)'],
+ testMatch: ['**/__tests__/**/*.test.[jt]s?(x)', '**/*.test.[jt]s?(x)'],

This worked locally and seems to be a reasonable change.
If the CI build works, is that an OK change for you?

[olance]

@Tobbe looks like the tests aren't being run... had you deactivated something at the PR level when they were stalling? ^^

[thedavidprice]

@olance This one seems to have been lost in the black hole of the holidays. Apologies! I'm taking a look at it now.

FYI: Tobbe just reworked the command rw setup tailwind and the PR merged yesterday. That's what's throwing the merge conflict.

[thedavidprice]

@olance OK, I just attempted to merge this with main and handle both conflicts and updates from a recent PR — admittedly it was a hurried process and I have not yet tested the results. But the CI is now running again.

@Tobbe just did a huge lift to correct multiple types of path issues. See #1648 I attempted to confirm the changes, now in main, are now reflected in this PR. Please assume I missed things @olance and scrutinize my merge against Tobbe's PR. Especially take a look at the "Add CSS Imports" task and respective code.

The tests you've added would be really helpful. You might have some other test ideas based on Tobbe's path work as well.

Keep me posted as to what you'd like to do from here. Thanks again!

[AntonioMeireles]

@thedavidprice, @olance

FWIW unless i'm wrong this isn't needed anymore. according to upstream it seems that @tailwindcss/ui not only is tailwindcss v1 specific but also isn't needed anymore with tailwindcss v2 . vd here and (from twitter)

The tailwindcss/ui plugin is not compatible with Tailwind CSS v2.0, check out the upgrade documentation on Tailwind UI for more details:https://t.co/4Amax9Etxd

Will try to remember to add a note about this to the Tailwind docs.

— Adam Wathan (@adamwathan) November 19, 2020

[thedavidprice]

@AntonioMeireles yes, indeed! It's way up in the thread now, but based on that change Olivier was going to take a different approach here. See above:
#1408 (comment)

Wanted to see if I could help get that going again. But understood either way.

[AntonioMeireles]

ah, ok. missed that bit. OTOH, and while on this not too sure if that is best approach from a UX and from a mantainability PoV... preferable to ship with a "recommended"/close to tip way and allow a plugable / separate 'legacy one' if it really needs to be (i.e. being clearly another option from cli and not just an install flag) with clear warnings that is not that well mantained/etc. otherwise when one bumps postcss/webpack we'll still have a legacy tail to grasp / handle [one completelly EoLed upstream]

(/me is just thinking loud)

[olance]

Hey there, long time no see 😅

@thedavidprice yeah I think it got kinda "lost", and I became extra busy at work, and with TailwindUI 2 out in the middle of that and the tests failing/stalling mysteriously this PR became much longer and heavier than anticipated, for something that's supposed to be quite simple in the end...

Now that time has passed, and following @AntonioMeireles lead, what do you think about simplifying this to just a v2 install? People wanting v1 can do it manually I guess...

[thedavidprice]

@olance Hey there! Understood how things go and never any expectations otherwise. Just never wanted to put you in the spot of waiting on me.

So, about this PR... There have been a lot of changes to the Setup command itself as well as several iterations on setup tailwind. Turns out we all stumbled into more complexity than expected, which turned into many more hours of effort than I ever would have expected. If you do want to move something forward, I'd definitely recommend closing this PR and starting anew.

That said, I'm not sure about either "if" or "how" to proceed. We have completed step 1 of the new Setup command: move existing, applicable commands under rw setup [command], keeping everything as is. Step 2 involves some re-structuring of the commands themselves — see the original proposal here. Eventually we want to pass a set of commands like rw setup deploy-netlify auth-supabase tailwind i18n, which becomes a way to quickly config and init a starter project. However, what this means is that we'll need to figure out how to handle flags, e.g. --force. Effectively any flag I add to that set of commands needs to either a) apply to all the commands and/or b) be unique to only one command. There has been an idea to create a syntax like rw setup ... tailwind--ui ... to effectively associate flags with a command, but I'm not sure if this is a good idea (yet). (E.g. how would we explain that in command-line help or docs?)

Specific to this PR, I'm not sure if it makes sense to add Tailwind UI as a flag given that we don't know how the Setup command will ultimately handle flags. The other option would be to create a separate command, e.g. rw setup tailwind-ui, but do you think that makes sense given how much overlap there already is with setup tailwind in terms of packages and config? E.g. would we just then be creating more work to make the two commands compatible when run separately and in any order?

Now having written all the above, I think the highest priority here is not the Tailwind UI but helping finish the structure of the Setup command itself (along with the current commands, which can only be run one at a time right now).

Welcome any/all thoughts. And, of course, no expectations or pressure.

[olance]

Hey! Thanks for the big update! 🙂

Il with you on the priority. Given all that's been going on, focus more energy on this seems wasted/not worth it. I'm OK to close the PR.
One thing though: I think I had come up with a nice test structure here for the commands - does it still make sense after the big revamp you mentioned?

Regarding this:

Eventually we want to pass a set of commands like rw setup deploy-netlify auth-supabase tailwind i18n, which becomes a way to quickly config and init a starter project. However, what this means is that we'll need to figure out how to handle flags, e.g. --force.

I used a tool named phpbrew recently and i loved how easy it was to do exactly that kind of quick setup (but for configure options/libs to be built with the requested PHP version):

For instance to build & install PHP 7.3 with xdebug and json support:

phpbrew install 7.3 +debug +json

In our case, that could be:

rw setup +deploy:netlify +auth:supabase +tailwind +i18n

(Yeah I find the : fancier!)

Then, if you need flags, you can use global ones after setup and package-specific ones after the package name:

rw setup --force +deploy:netlify +auth:supabase +tailwind --ui +i18n --fr

What do you think?

[jtoar]

Wanted to mention this issue on the yargs repo about enabling exactly this kind of structure for the setup command: yargs/yargs#1821. Just to show that it might be implemented one day. But @olance I like that style a lot; the global flags for the command coming before the subcommands also makes sense and is how deno does it.

[jtoar]

To show what we're up against, here's a step-by-step description of tweaking setup to better accept @olance's command string. I made it so that setup just returns argv. So setup looks like this:

export const command = 'setup <commmand>'
export const description = 'Initialize project config and install packages'
export const handler = (argv) => console.log(argv)

argv is what the yargs-parser gives us from what the user enters at the command line. Let's see what yargs does by default with @olance's command string

yarn rw setup --force +deploy:netlify +auth:supabase +tailwind --ui +i18n --fr

{
  _: [ 'setup', '+tailwind' ],
  force: '+deploy:netlify',
  ui: '+i18n',
  fr: true,
  '$0': 'rw',
  commmand: '+auth:supabase'
}

There's a lot of problems with the way this is being parsed. For one, --force is receiving +deploy:netlify as it's argument (instead of it just being a boolean, like force: true)—we can quickly fix that by adding a builder to setup that specifies the options it takes:

 export const command = 'setup <commmand>'
 export const description = 'Initialize project config and install packages'
+export const builder = (yargs) => yargs.option('force', { type: 'boolean' })
 export const handler = (argv) => console.log(argv)

{
  _: [ 'setup', '+auth:supabase', '+tailwind' ],
  force: true,
  ui: '+i18n',
  fr: true,
  '$0': 'rw',
  commmand: '+deploy:netlify'
}

Better, but +deploy:netlify's in command while everything else is in _ (only setup should really be there). Let's try changing setup's command to accept variadic positional arguments:

-export const command = 'setup <commmand>'
+export const command = 'setup [commmand..]'
 export const description = 'Initialize project config and install packages'
 export const builder = (yargs) => yargs.option('force', { type: 'boolean' })
 export const handler = (argv) => console.log(argv)

{
  _: [ 'setup' ],
  force: true,
  ui: '+i18n',
  fr: true,
  '$0': 'rw',
  commmand: [ '+deploy:netlify', '+auth:supabase', '+tailwind' ]
}

Much better! But we still haven't addressed that ui, an option to +tailwind, is receiving +i18n as it's argument; +i18n should really be in command, and ui should be true like force.

This is where we start to run up against the limit of what we can do just by tweaking the yargs exports (command/description/builder/handler). So where do we go from here?

make it so that the builder knows all the subcommand's options

Why don't we just add ui to builder like we did with force? That would make it so that it parses as true. But now we have to ask: how do we know --ui is for +tailwind? There's nothing in argv that says so; we'd just have to know in advance. And the problem there is we can't have subcommands that have the same options. This is a huge problem with --force, which pretty much everything accepts. How do we know what you wanted to overwrite and what you didn't?

write some gnarly middleware to reparse argv before it hits the handler

Yargs has the concept of middleware; it runs before argv hits the handler, so we could do something kind of similar to afterQuery in a Cell, and majorly restructure argv. But that still doesn't address the option clashing issue.

organizing arguments into nested objects

Quoting from the Yargs docs,

When you use dots (.s) in argument names, an implicit object path is assumed. This lets you organize arguments into nested objects.

To see this in action, let's tweak @olance's command string and run it:

-yarn rw setup --force +deploy:netlify +auth:supabase +tailwind --ui +i18n --fr
+yarn rw setup --force --deploy netlify --auth supabase --tailwind.ui --i18n.fr

  _: [ 'setup' ],
  force: true,
  deploy: 'netlify',
  auth: 'supabase',
  tailwind: { ui: true },
  i18n: { fr: true },
  '$0': 'rw'
}

That actually looks amazing! We can just iterate through the object properties; the subcommands are already organized with their arguments.

To really drive the point home, let's add an option to deploy, like --data-migrate:

-yarn rw setup --force --deploy netlify --auth supabase --tailwind.ui --i18n.fr
+yarn rw setup --force --deploy netlify --deploy.data-migrate --auth supabase --tailwind.ui --i18n.fr

{
  _: [ 'setup' ],
  force: true,
  deploy: [ 'netlify', { 'data-migrate': true }, { dataMigrate: true } ],
  auth: 'supabase',
  tailwind: { ui: true },
  i18n: { fr: true },
  '$0': 'rw'
}

That looks near perfect. And our setup command looks like this; it's as vanilla as it gets (I even took out builder and force still parses correctly)!

export const command = 'setup'
export const description = 'Initialize project config and install packages'
export const handler = (argv) => console.log(argv)

Did we have to repeat --deploy? Yes, and that might be a shortcoming of this syntax.

conclusion

So I'm not sure what to do from here; I just wanted to illustrate that there's a "native" syntax yargs understands. Whether or not we like that syntax is the question now.

As @mojombo says, computers should do what we want, so we should probably just pick the syntax we like and code for it. But with the dot syntax, it's a downhill battle/most of the work's already done.

[olance]

Woa thanks for the analysis @jtoar ! :)

I must say I'm not a fan of this dotted syntax, it looks weird for what we'd want to do... like, does --tailwind.ui set up TailwindUI or Tailwind with Tailwind UI?
The end result for us internal developers looks great, but the "API" for the user is a bit too vague to my liking.

Can't dive into it tonight, but when I see something like this:

I'm wondering: couldn't we replace hideBin with a function of our own that would preprocess process.argv from whatever CLI format we fancy into the dotted syntax you showed?
That would give us the best of both worlds, with only a little extra work?

[olance]

FYI I'll explore the above tomorrow!

[olance]

@jtoar There you go! 🙂

I've setup a PoC repo here: https://github.com/olance/poc-redwood-yargs-parsing

With the following params:

--force +deploy:netlify --migrate-data +auth:supabase +tailwind --ui --version 2 +i18n --lang=en,fr

The argv object out of yargs is what you obtained using the dotted notation:

{
  force: true,
  deploy: [ 'netlify', { 'migrate-data': true }, { migrateData: true } ],
  auth: 'supabase',
  tailwind: { ui: true, version: 2 },
  i18n: { lang: 'en,fr' },
}

The code is pretty simple, it only walks through the args and transforms them to the dotted notation yargs supports before it is parsed:

const argv = yargs(processArgs(hideBin(process.argv))).argv

function processArgs(argv) {
  console.log("Input argv:", argv)

  let args = argv.reverse()
  let currentScope = null
  let newArgs = []

  while (args.length > 0) {
    let arg = args.pop()

    // Beginning a new scope?
    if (arg[0] === SCOPE_TOKEN) {
      // Extract scope name.
      // That would be 'deploy' for '+deploy:netlify' and 'tailwind' for '+tailwind'
      // TODO: should we handle multi-level scopes, i.e. +deploy:gcp:cloud-run / +deploy:gcp:appengine? (seems unlikely)
      const scopeComponents = arg.substr(1).split(SCOPE_VALUE_SEPARATOR)
      currentScope = scopeComponents[0]

      // If it's a "parameterized" scope ('+deploy:netlify') we need to issue an arg for the scope value
      if (scopeComponents.length === 2) {
        newArgs.push(`--${currentScope}=${scopeComponents[1]}`)
      }

      continue
    }

    // Down here means we're _within_ a scope, possibly null (global scope)
    const argName = arg.substr(2) // Assuming args always start with '--'
    let newArg = '--'

    if (currentScope !== null) {
      newArg += `${currentScope}.`
    }

    // We now have newArg ==
    // -> '--force' for a global '--force' flag
    // -> '--tailwind.ui' for something like '+tailwind --ui'
    // -> '--i18n.lang=en,fr' for something like '+i18n --lang=en,fr'
    newArg += argName

    // If the arg needs a value and it's been passed like '--lang en,fr' it's not covered by the above, we need to look
    // ahead and consume the next arg if needed
    const nextArg = args[args.length - 1]
    if (argName.indexOf('=') < 0 && nextArg[0] !== SCOPE_TOKEN && nextArg.substr(0, 2) !== '--') {
      newArg += `=${args.pop()}`
    }

    newArgs.push(newArg)
  }

  console.log('Transformed argv:', newArgs)

  return newArgs
}

[olance]

Maybe it's time to take this discussion to some other place?

[thedavidprice]

moved "test structure" discussion over here: #1913

[thedavidprice]

moved "setup command" discussion over here #1930

[thedavidprice]

Thanks, all! I'm closing this one out. It might not have been merged, but it's definitely created a lot of momentum.