Validates `tailwindcss` if Tailwind is used #2465

infomiho · 2025-01-23T14:25:44Z

For more details: #2464

infomiho · 2025-01-23T14:28:11Z

waspc/src/Wasp/AppSpec.hs

@@ -149,10 +148,6 @@ resolveRef spec ref =
    $ find ((== refName ref) . fst) $
      getDecls spec

-doesConfigFileExist :: AppSpec -> Path' (Rel WaspProjectDir) File' -> Bool


I needed the information if Tailwind is used (which is computed using doesConfigFileExist) before we have the AppSpec so I pulled it out of here and rewritten it so it doesn't need the AppSpec.

How is it that you needed it before?
Are we getting into a situtation where we need a lot of info from AppSpec before AppSpec exists, pointing at some potential design issue, or do you think this is ok?

infomiho · 2025-01-23T14:28:33Z

waspc/src/Wasp/Generator/ConfigFile.hs


-- | Establishes the mapping of what config files to copy and where from/to.


infomiho · 2025-01-23T14:29:39Z

waspc/src/Wasp/Generator/ConfigFile.hs

+  doesConfigFileExist tailwindConfigFile
+    && doesConfigFileExist postcssConfigFile
+  where
+    doesConfigFileExist :: Path' (Rel WaspProjectDir) File' -> Bool


This is only used for isTailwindUsed so I inlined it even though it might be useful in the future - let's pull it out when it becomes necessary?

If you know it is a general utility, I would pull it out immediately, since then it is easier for somebody else to see it there and use it. If it was more tied down to this specific place then mabye not, but it seems quite ready to isolate right now, and I can easily imagine somebody else will want to check if certain congfi file exists, so yeah, let's go for it right now.

infomiho · 2025-01-23T14:30:25Z

waspc/src/Wasp/Generator/ExternalConfig/PackageJson.hs

-validatePackageJson :: P.PackageJson -> [ErrorMsg]
-validatePackageJson packageJson =
+validatePackageJson :: Bool -> P.PackageJson -> [ErrorMsg]
+validatePackageJson isTailwindUsed packageJson =


I had to thread through the isTailwindUsed info unfortunately. Some sort of Reader would be useful here, but I didn't want to over-engineer it.

I get what you mean, and I agree it would be much going for Reader now.
That said, having just a Bool like this is not very nice, what if we add one or two more conditionals like this, would we have 2 more arguments? I would immediately go for grouping this into a record, even it if has just one field for now. Might be called something like ValidationConfig or ValidationPreconditions or ValidationContext (I like that one!) or whatever you think is the best name. That will also make moving to Reader easier in the future if we want it, since you would just move that record into Reader. But it will also make this function immediately more readable and extendable.

infomiho · 2025-01-23T14:34:49Z

waspc/src/Wasp/Project/Analyze.hs

@@ -61,25 +66,25 @@ analyzeWaspProject waspDir options = do
          analyzeWaspFile waspDir prismaSchemaAst waspFilePath >>= \case
            Left errors -> return (Left errors, [])
            Right declarations ->
-              EC.analyzeExternalConfigs waspDir (getSrcTsConfigInWaspProjectDir waspFilePath) >>= \case
+              EC.analyzeExternalConfigs isTailwindUsed waspDir (getSrcTsConfigInWaspProjectDir waspFilePath) >>= \case


I wasn't sure if I wanted to thread the configFiles all the way down to validatePackageJson and the call the G.CF.isTailwindUsed check. Since validatePackageJson doesn't need the list of configs, I decided not to.

Hm maybe they don't need it all, but this function is called analyzeExternalConfigs and now it takes a bool, waspDir, and then a specific config file (srcTsConfig). That is now quite messy, because you could look at those args as:
arg#1: info about config file
arg#2: context
arg#3: config file

Usually in Haskell we aim for "context" args at start, and then "core" args after that.
Since this function is about config files, any amount of them, then I would also find it more natural that it takes a list or record of config files, note one per argument.

So what I would consider is turning this function's signature into :: WaspDir -> ConfigFiles -> ReturnType (I used a bit pseudo names here).

And it's not a problem if it takes a bit too much info -> it will analyze what it needs to analyze. We don't want to guess how it will analyze them, that is implementation detail.

infomiho · 2025-01-23T14:35:21Z

waspc/src/Wasp/Generator/ExternalConfig/PackageJson.hs

  where
    validate = validateDep packageJson

+    tailwindValidations =
+      [ validate ("tailwindcss", show tailwindCssVersion) IsListedWithExactVersion | isTailwindUsed


If Tailwind is used, validate that the tailwindcss package is present in the package.json with the correct version.

Signed-off-by: Mihovil Ilakovac <[email protected]>

Martinsos

All makes sense, but I believe we can improve data flow / function signatures a bit, left comments on that!

Martinsos · 2025-01-23T21:09:08Z

waspc/src/Wasp/Generator/ExternalConfig/PackageJson.hs

-validatePackageJson :: P.PackageJson -> [ErrorMsg]
-validatePackageJson packageJson =
+validatePackageJson :: Bool -> P.PackageJson -> [ErrorMsg]
+validatePackageJson isTailwindUsed packageJson =


I get what you mean, and I agree it would be much going for Reader now.
That said, having just a Bool like this is not very nice, what if we add one or two more conditionals like this, would we have 2 more arguments? I would immediately go for grouping this into a record, even it if has just one field for now. Might be called something like ValidationConfig or ValidationPreconditions or ValidationContext (I like that one!) or whatever you think is the best name. That will also make moving to Reader easier in the future if we want it, since you would just move that record into Reader. But it will also make this function immediately more readable and extendable.

Martinsos · 2025-01-23T21:31:57Z

waspc/src/Wasp/Project/Analyze.hs

@@ -61,25 +66,25 @@ analyzeWaspProject waspDir options = do
          analyzeWaspFile waspDir prismaSchemaAst waspFilePath >>= \case
            Left errors -> return (Left errors, [])
            Right declarations ->
-              EC.analyzeExternalConfigs waspDir (getSrcTsConfigInWaspProjectDir waspFilePath) >>= \case
+              EC.analyzeExternalConfigs isTailwindUsed waspDir (getSrcTsConfigInWaspProjectDir waspFilePath) >>= \case


Hm maybe they don't need it all, but this function is called analyzeExternalConfigs and now it takes a bool, waspDir, and then a specific config file (srcTsConfig). That is now quite messy, because you could look at those args as:
arg#1: info about config file
arg#2: context
arg#3: config file

Usually in Haskell we aim for "context" args at start, and then "core" args after that.
Since this function is about config files, any amount of them, then I would also find it more natural that it takes a list or record of config files, note one per argument.

So what I would consider is turning this function's signature into :: WaspDir -> ConfigFiles -> ReturnType (I used a bit pseudo names here).

And it's not a problem if it takes a bit too much info -> it will analyze what it needs to analyze. We don't want to guess how it will analyze them, that is implementation detail.

Martinsos · 2025-01-23T21:33:23Z

waspc/src/Wasp/Project/Analyze.hs

  Psl.Schema.Schema ->
  [AS.Decl] ->
  IO (Either [CompileError] AS.AppSpec, [CompileWarning])
-constructAppSpec waspDir options externalConfigs parsedPrismaSchema decls = do
+constructAppSpec waspDir options externalConfigs configFiles parsedPrismaSchema decls = do


What confuses me a bit is that this function takes both external configs and congif file relocators. Actually those two kind of make sense -> I guess it takes them, and then rules how to relocate them, I am not up to date with this piece of code completely so I am not quite sure -> but then for relocators you use configFiles name and that confuses me absolutely, because you said relocators but now it is files.

Martinsos · 2025-01-23T21:34:19Z

waspc/src/Wasp/Project/ExternalConfig.hs

@@ -23,11 +23,12 @@ data ExternalConfigs = ExternalConfigs
  deriving (Show)

 analyzeExternalConfigs ::
+  Bool ->


In general, seeing a Bool like this, where I can't tell from signature at all what it could be, is a bit suspicious. I commented more above why this one is probably not the right choice here.

Martinsos · 2025-01-23T21:35:46Z

waspc/src/Wasp/Project/ExternalConfig/PackageJson.hs

@@ -18,11 +18,11 @@ import Wasp.Project.Common
  )
 import qualified Wasp.Util.IO as IOUtil

-analyzePackageJsonFile :: Path' Abs (Dir WaspProjectDir) -> IO (Either [String] P.PackageJson)
-analyzePackageJsonFile waspProjectDir = runExceptT $ do
+analyzePackageJsonFile :: Bool -> Path' Abs (Dir WaspProjectDir) -> IO (Either [String] P.PackageJson)


We should probably rework this signature also, with similar approach as to how I suggested in other comments.

Martinsos · 2025-01-23T21:38:22Z

waspc/src/Wasp/Generator/ConfigFile.hs

+  doesConfigFileExist tailwindConfigFile
+    && doesConfigFileExist postcssConfigFile
+  where
+    doesConfigFileExist :: Path' (Rel WaspProjectDir) File' -> Bool


If you know it is a general utility, I would pull it out immediately, since then it is easier for somebody else to see it there and use it. If it was more tied down to this specific place then mabye not, but it seems quite ready to isolate right now, and I can easily imagine somebody else will want to check if certain congfi file exists, so yeah, let's go for it right now.

Martinsos · 2025-01-23T21:39:11Z

waspc/src/Wasp/AppSpec.hs

@@ -149,10 +148,6 @@ resolveRef spec ref =
    $ find ((== refName ref) . fst) $
      getDecls spec

-doesConfigFileExist :: AppSpec -> Path' (Rel WaspProjectDir) File' -> Bool


How is it that you needed it before?
Are we getting into a situtation where we need a lot of info from AppSpec before AppSpec exists, pointing at some potential design issue, or do you think this is ok?

Validates tailwindcss if Tailwind is used

1af064b

infomiho mentioned this pull request Jan 23, 2025

Add tailwindcss as an explicit dependecy and validate that it exists in user's package.json #2464

Closed

3 tasks

infomiho commented Jan 23, 2025

View reviewed changes

infomiho added 3 commits January 23, 2025 15:35

e2e tests

1df40ff

Signed-off-by: Mihovil Ilakovac <[email protected]>

Fixes todoApp

f1a8075

Update headless tests

b6fae16

Martinsos requested changes Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validates `tailwindcss` if Tailwind is used #2465

Validates `tailwindcss` if Tailwind is used #2465

infomiho commented Jan 23, 2025

infomiho Jan 23, 2025

Martinsos Jan 23, 2025

infomiho Jan 23, 2025

infomiho Jan 23, 2025

Martinsos Jan 23, 2025

infomiho Jan 23, 2025

Martinsos Jan 23, 2025

infomiho Jan 23, 2025

Martinsos Jan 23, 2025

infomiho Jan 23, 2025

Martinsos left a comment

Martinsos Jan 23, 2025

Martinsos Jan 23, 2025

Martinsos Jan 23, 2025

Martinsos Jan 23, 2025

Martinsos Jan 23, 2025

Martinsos Jan 23, 2025

Martinsos Jan 23, 2025


		-- \| Establishes the mapping of what config files to copy and where from/to.

Validates tailwindcss if Tailwind is used #2465

Are you sure you want to change the base?

Validates tailwindcss if Tailwind is used #2465

Conversation

infomiho commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martinsos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Validates `tailwindcss` if Tailwind is used #2465

Validates `tailwindcss` if Tailwind is used #2465