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?

infomiho · 2025-02-03T16:16:59Z

@Martinsos could you take another look? I've read through your comments and took another stab at the implementation, let me summarize my thought process and what I did.

First, I'll jump into my thoughts about the way we support Tailwind and then I'll go into the ExternalConfigAnalysisContext

Before we can construct the AppSpec, we need to analyze/validate the package.json and tsconfig.json files because they need to be valid before we continue with generating the project. (We call these externalConfigs)
Before we can analyze the package.json we need to know if Tailwind is used because we want to validate Tailwind CSS only if it's used.
Users enable Tailwind CSS implicitly by defining tailwind.config.cjs and postcss.config.cjs in their project dir.
We devised a general way of relocating user defined configs, you specify the source path and the target path. We use this only for Tailwind related config files. (We call these configFiles with only Tailwind and PostCSS configs inside)

We were going for the magical DX where we install Tailwind dep in the background when we detect the Tailwind config files. Users didn't have to install the npm package themselves. Given the new validation rules for the tailwindcss dep (which we have to have due to peer deps issues) users need to have the tailwindcss dep in their package.json anyways. So, Wasp is not really doing anything anymore. When we migrate to Tailwind 4, I propose we make Tailwind a user-land feature: #2483

In general, I feel like the mechanism of configuring your Wasp project by Wasp detecting presence of certain files like tailwind.config.cjs and postcss.config.cjs is kinda confusing. Based on our experience with users wanting to have more control, I believe the more explicit configuration the better. So, I think we should maybe get rid of the ConfigFileRelocator concept all together.

Given that I think we should get rid of ConfigFileRelocator as a concept, I've made parts of it less general and made it specific to Tailwind. It's much more clear to me what is now going on in the AppSpec, there is no longer configFiles but tailwindConfigFilesRelocators - super specific. Let me know what you think.

Also, I liked your suggestion to have a ...Context for the external config files analysis, so I did just that. All the related functions now use the ExternalFilesAnalysisContext object which made it much easier to implement.

I've also tried to introduce the AppSpecConstructionContext so maybe make it easier in the future to introduce new into without introducing new function params. I'm not sure about this one, looks okay to me, let me know what you think.

Martinsos

@infomiho I think this is a step in right direction, but you might have went a bit too far with it.

The point of the "context" was to enable passing around some data without having to pass every single argument individually, where arguments were really implementation details of other, deeper layers of logic, specifically specific config files like tailwind and so on -> top-level external config files fuction doesn't want to list every congif file as its argument, but take them as one big data chunk that it can later dissect and delegate to smaller functions if it wants.

That was kind of the idea. Also you put in some stuff that might not even belong there, like waspDir -> I am not sure about that yet though. But I would say, look at the idea of "context" as a way to wire stuff through while controlling amount of information that different layers of logic need to know about, and also look at it more from the "site of call" then from its own position -> the context shouldn't be a standalone thing that is used in other places, conceptually, unless it is indeed super clear and standalone concept (which I don't think is true here), but instead should be kind of dependant / modeled by the usage site / consumer, in this case your validation logic in ExternalCOnfig module.

I commented more in the individual comments, read them all before you start acting on any of them, to get the full picture.

We can also discuss further if needed, if I was unclear about something -> I have a feeling here what is right / wrong, but I am a bit vague in telling you exactly what it should look like, because I don't have a clear picture myself yet, I would have to get my hands dirtier if I wanted to figure that out exactly.

Martinsos · 2025-02-11T07:26:43Z

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

+    WaspProjectDir,
+  )
+
+data ExternalConfigAnalysisContext = ExternalConfigAnalysisContext


When I was suggesting implementing ValidationContext, I was kind of expecting it to be coming from the side of validation code, where it is used -> tailored for what it needs. It seems to me you however went in a bit different direction -> you created a general context object that contains information that somebody analysing external config files might need. Which means you are guess, from the perspective of ExternalConfig, what the analysis of them might need. That might befine, but it is a bit unusual / different, it feels like you might be trying to guess too much about their usage, and therefore that will result in different parts of the code all using this Context while it is not really tailored for any of them specifically.

Just to be clear, what I imagined was that you have ValidationContext which has just isTailwindUsed in it and that is it, IF that is all you need in the validation part of the code.

waspDir being here for example is a bit weird. That is such a super general information, it is not related to external configs only and I could imagine most of the funcitons already having it in their context, meaning you are likely passing it twice. While _srcTsConfigPath feels better. Relocators feel also a bit weird since it seems to me they are more often the object of manipulation than a contextual thing -> I might be wrong on that one though, I could see how they could be part of the context. But that is the issue -> here we are guessing what somebody might need. I don't think ExternalConfig module should be troubling itself with that, it feels wrong.

Martinsos · 2025-02-11T10:43:07Z

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

  )
+import qualified Wasp.Project.ExternalConfig.ExternalConfigAnalysisContext as ECC
 import qualified Wasp.Util.IO as IOUtil
 import Wasp.Util.Json (parseJsonWithComments)

 analyzeSrcTsConfigFile ::


So here, I don't think you gained much by using the Context data type -> arguments passed seemed not likely to change, and were reasonable for the situation -> it takes waspDir, and it takes srcTsConfigFile, which is the very object of this function. Now you put the main argument, srcTsConfigFile, into Context, which is weird. If anything should go into context, it is waspDir only, but even that is not needed in this case I think.

Martinsos · 2025-02-11T10:44:46Z

waspc/src/Wasp/Generator/TailwindConfigFile.hs


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


This comment is outdated then since this is only about tailwind now?

Martinsos · 2025-02-11T10:47:10Z

waspc/src/Wasp/Generator/TailwindConfigFile.hs

-isTailwindUsed spec =
-  doesConfigFileExist spec tailwindConfigFile
-    && doesConfigFileExist spec postcssConfigFile
+isTailwindUsed :: [CF.ConfigFileRelocator] -> Bool


This list of config file relocators is a bit confusing argument here, compared to previous app spec -> before I would get app spec and this function would tell me if tailwind is used. OK. Now, it gets a list of config file relocators which is kind of an impelmentation detail and then tells me based on that if it is used. I guess then it assumes that tehse are all the confg file relocators there are out there? I mean, it is just a list, it could contain anything, you didn't specify any expectations here. Feels like some additional instructions are missing, if used like this. Also feels like maybe this is not the rigth way to go, but I am not sure without digging deeper.

Martinsos · 2025-02-11T10:52:34Z

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


-validatePackageJson :: P.PackageJson -> [ErrorMsg]
-validatePackageJson packageJson =
+validatePackageJson :: ECC.ExternalConfigAnalysisContext -> P.PackageJson -> [ErrorMsg]


Ok yeah, this context makes kind of sense here. Providnig waspDir via it still feels a bit weird, but the rest is ok I believe. But here we are in ExternalConfig module, and that ExternalConfigAnalysisContext is really crafted directly for this place of usage. Which i think makes sense. But then I wouldn't use it in othe rplaces also, especially oustide of ExternalConfig module, because it is too specific for those places. I would even consider moving the data type directly here into this file, and have it be local here in a way.

Martinsos · 2025-02-11T10:58:36Z

waspc/src/Wasp/Project/Analyze.hs

+                            _decls = declarations
+                          }
+
+data AppSpecConstructionContext = AppSpecConstructionContext


I wouldn't call this Context -> Context is supporting data for the "main" data. Well, context. This is the "meat", all that is needed to construct the app spec. Context might be just part of it -> waspDir, compileOptions, ... . Maybe everything expect for decls? Because decls are the main part for sure. Maybe also prisma schema should not be in context.
But here you don't gain much with this data type all together, so I probably wouldn't do it at all, no data AppSpecConstructionContext -> it is anyway not something you will be sending around. Let's put it this way -> there isn't enough reason to create it. If there was, it would be clearer.

Martinsos · 2025-02-11T12:02:38Z

Before we can construct the AppSpec, we need to analyze/validate the package.json and tsconfig.json files because they need to be valid before we continue with generating the project. (We call these externalConfigs)

But if you need to validate the package.json and tsconfig.json files before generating the project, you could also do that after the construction of AppSpec and still before the generation of the project, that is our Validate.hs step. Or is there some other reason to do it before?

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

Merge branch 'main' into miho-validate-tailwindcss

c2038c4

infomiho mentioned this pull request Feb 3, 2025

Migrate to Tailwind 4 #2483

Open

infomiho added 2 commits February 3, 2025 16:49

PR comments

37ebc94

Rename AppSpecConstructionContext

282959a

infomiho self-assigned this Feb 5, 2025

Martinsos reviewed Feb 11, 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

infomiho commented Feb 3, 2025

Martinsos left a comment

Martinsos Feb 11, 2025

Martinsos Feb 11, 2025

Martinsos Feb 11, 2025

Martinsos Feb 11, 2025

Martinsos Feb 11, 2025

Martinsos Feb 11, 2025

Martinsos commented Feb 11, 2025


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


		-- \| Establishes the mapping of what config files to copy and where from/to.
		-- 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

infomiho commented Feb 3, 2025

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

Martinsos commented Feb 11, 2025

Validates `tailwindcss` if Tailwind is used #2465

Validates `tailwindcss` if Tailwind is used #2465