-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Userland files validation #2297
Conversation
2e16552
to
3ebe67a
Compare
Co-authored-by: Martin Šošić <[email protected]>
8d73478
to
fbdf314
Compare
fbdf314
to
aad26e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@infomiho looking good! I think all the big stuff is done, there is a couple of smaller things left that I commented on, but major stuff is good.
import Wasp.Generator.Common (prismaVersion) | ||
import Wasp.Generator.WebAppGenerator.Common (reactRouterVersion) | ||
|
||
validatePackageJson :: P.PackageJson -> IO [String] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's maybe add type ErrorMsg = String
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thought on top of this: often, in our code, we just use Strings as error messages. General advice is that this is not good, you should use more structured types, but the truth is, for our needs this seems to be right at the moment.
However, it is still much nicer to see ErrorMsg
in output type than String
because it tells me what it is, which is why type ErrorMsg = String
.
Now, sometimes it is nice to call it more specifically: type CompilerError = String
, or type ParsingError = String
, but in many parts of the code we don't want to bother iwth that and just doing type ErrorMsg = String
is enough. But that means we have a lot of places where we do the same thing: define type ErrorMsg = String
. So I wonder if we should just have central type ErrorMsg = String
in some Wasp.Utils
or something like that? We don't have to do this in this PR, but wanted to mention this now since I was talking about this. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll turn this line of thinking into an issue 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
analyzePackageJsonFile waspProjectDir = runExceptT $ do | ||
packageJsonFile <- ExceptT findPackageJsonFileOrError | ||
packageJson <- ExceptT $ readPackageJsonFile packageJsonFile | ||
ExceptT $ validateToEither validatePackageJson packageJson |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking at this call of validateToEither here to figure out if we might not need it, since it seemed to me it is oddly specific function and we could probably replace it with something nicer.
I was looking to do something like following instead:
ExceptT $ <sometransformation> $ validatePackageJson packageJson
So validatePackageJson packageJson
gives us IO [String]
. If this array is not empty , we want to shortcircuit the computation with this array sinced it is a list of errors. We could do this with following function:
\errors -> if null errors then Right () else Left errors
I guess we could name it like this:
errorsListToEither [] = Right ()
errorsListToEither _ = Left errors
And then we map it:
ExceptT $ errorsListToEither <$> validatePackageJson packageJson
actually, fmap it, since it doesn't operate in IO
.
Although, I noticed in another comment that validatePackageJson packageJson
doesn't even have to be IO
, so that will also change things here, but I think the idea will remain the same.
Another approach which I have only a rough hint it might work but you can give it a try and see if it makes sense or not, buit I think ExceptT
might be instance of ErrorMonad (or is it MonadError?), therefore supporting stuff like throw
(or is it throw<something>
?) and try
or something like it. If that is so, you might be able to just do something like
case validatePackageJson packageJson of
[] -> pure packageJson
errors -> throwError errors
I assumed here that you made validatePackageJson
to not be IO
.
Although throwError
here you could probably just replace with ExceptT . pure . Left
, that is what it will be doing in the background I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with something super simple:
case validateTsConfig tsConfig of
[] -> return tsConfig
errors -> throwError errors
Thanks for the tip, it makes sense to inline it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok perfect! I wrote a lot above in the comment in case train of thought is useful, but yeah this is the optimal solution at the end.
Co-authored-by: Martin Šošić <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@infomiho I think this is it! Left couple of small comments but none require any changes I believe, give them a quick look and them merge when happy.
@@ -86,8 +96,8 @@ tsconfigInWaspProjectDir = [relfile|tsconfig.json|] | |||
|
|||
findFileInWaspProjectDir :: | |||
Path' Abs (Dir WaspProjectDir) -> | |||
Path' (Rel WaspProjectDir) File' -> | |||
IO (Maybe (Path' Abs File')) | |||
Path' (Rel WaspProjectDir) (File file) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch :D!
Closes #2169
Validates
package.json
It loads the JSON and checks versions of deps: Wasp SDK, Prisma and React Router DOM.
Validates
tsconfig.json
It loads the
tsconfig.json
which is JSON with comments (uses Node.js's eval to get proper JSON). It checks all the fields.