-
-
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
Separate user code into client and server directories #753
Conversation
@@ -6,18 +6,18 @@ where | |||
import Control.Monad.Except (throwError) |
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 introduced a lot of changes to this file. The best way to review it is probably to avoid looking at the diff altogether.
The main (non-refactor) changes are:
- We now create two separate directories for user code: client and server
- Since most of the project scaffolding doesn't depend on anything, we now create a new project mainly by copying a skeleton from our templates and dynamically generate files only when necessary (i.e., for
main.wasp
file which depends onProjectInfo
).
maybeDotEnvServerFile <- findDotEnvServer waspDir | ||
maybeDotEnvClientFile <- findDotEnvClient waspDir |
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.
@shayneczyzewski Do you think it makes sense to move .env
files into server
and client
directories? It might be a bit cleaner.
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.
Yeah, good thinking- I think it would probably make sense for them to be renamed .env
inside each respective directory.
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.
Sounds good to me also!
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.
We'll do this in a different issue, I'll make one before merging the PR.
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.
Did this issue get created BTW?
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.
Not yet, I usually go through all unresolved threads and create issues while waiting for the pre-merge CLI to do its thing.
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.
Done: #811.
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.
Hey @sodic I like the direction here, nice job! I wanted to take this on in a few passes, so this was my first. :)
At a high level, I think the direction makes sense. A few things I noted:
We may want to keep the.env
warning around.- You are right, that having those two very similar identifiers for compile options and AppSpec is a bit of a code smell for some reason. I assume it would feel worse when we add a "shared" folder. We could try to put them into some additional structure, or we could punt as a TODO and try to solve it when we add "shared". I will think more on this one.
- I will do more testing once the example app is updated. Initial new project issues were noted in a comment.
Can't wait to take a second pass, hopefully with some ideas on how to address that similar variable issue you noted. Will think on it and try a few things out. I'll also think about it in the context of multiple wasp files/dirs (so instead of having an explicit client and server, maybe we have a dynamic collection of files/dirs and they are just labeled client/server, for example).
maybeDotEnvServerFile <- findDotEnvServer waspDir | ||
maybeDotEnvClientFile <- findDotEnvClient waspDir |
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.
Yeah, good thinking- I think it would probably make sense for them to be renamed .env
inside each respective directory.
Co-authored-by: Shayne Czyzewski <[email protected]>
Love that thinking direction, and in generally thinking about this! Dream of modular Wasp is closer and closer :D! |
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.
Looking good @sodic ! Left some comments, nothing major.
The big question, I believe, is: what about shared code, that both client and server use?
Additional question: should we organize it all under some kind of src/ dir? I am not sure why I am even asking this, but it feels like it might make sense, I guess I have just seen it on so many places. But then, I can't find a good enough reason for it. I believe src/ is normally used to separate source code from other stuff like some very top lvl config, docs, readmes, ... .
But I guess they can just put all of this stuff under src/ if they wish. How would it look if we added src? I guess client/ and server/ would go under it, and .wasp would remain next to them. But .wasp is also source file! But it is more of a config though. Maybe .wasp also goes into src? Hm. Ok, just a bit of wild thinking, let me know what you think @shayneczyzewski @sodic .
@sodic checked your responses, sounds good, so ping me once you do the changes and are ready for final review! |
f6ec33b
to
6d29601
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.
@sodic checked comments, looking good!
The only non-trivial comment is the one about the name of analyzeWaspProject function.
Btw, what about questions I asked here?
#753 (review)
We discussed "src/" a bit, I think we concluded it might make sense t have it.
What about shared/?
Are those changes coming in this PR, I guess they should?
4ef31f8
to
8674887
Compare
Nice catch @shayneczyzewski, I fixed in the next commit. Btw, according to this comment, that globbing quirk should be fixed in next versions of cabal. |
c0c2480
to
0cb6369
Compare
mkError ctx msg = Left $ ER.mkEvaluationError ctx $ ER.ParseError $ ER.EvaluationParseError msg | ||
stripImportPrefix importPath = stripPrefix serverPrefix importPath <|> stripPrefix clientPrefix importPath | ||
serverPrefix = "@server/" | ||
clientPrefix = "@client/" |
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.
@Martinsos I believe this is the temporary solution we've settled on:
- Wasp requires users to import from
@client
and@server
, but works regardless of which one they pick 😅
I'll resolve this comment after I make an issue for 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.
Done: #810
@@ -10,7 +10,7 @@ import Wasp.Generator.ExternalCodeGenerator.Common (GeneratedExternalCodeDir) | |||
|
|||
getJsImportDetailsForExtFnImport :: | |||
-- | Path to generated external code directory, relative to the directory in which file doing the importing is. | |||
Path Posix (Rel (Dir a)) (Dir GeneratedExternalCodeDir) -> | |||
Path Posix (Rel a) (Dir GeneratedExternalCodeDir) -> |
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.
This was an error, there's no need to nest Dir
s.
extServerCodeDirInServerSrcDirP :: Path Posix (Rel C.ServerSrcDir) (Dir GeneratedExternalCodeDir) | ||
extServerCodeDirInServerSrcDirP = fromJust $ relDirToPosix extServerCodeDirInServerSrcDir |
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.
@Martinsos Is this safe? I do it in several other places.
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.
It will give runtime error if that value is Nothing (if its conversion from rel dir to posix failed). Since we are dealing with our paths here, and not user input, I think it is ok.
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 thought so, thanks for confirming!
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.
This is awesome @sodic, amazing work! I love how much nicer the dir structure feels with src
and having things split into client/server/shared (and importing with @client
etc 🔥). Awesome 👏🏻 And the ExceptT
usage to clean a few things up was great.
Just a few small questions/comments, but non are blockers and feel free to ignore them honestly. I think this looks ready.
One thing I would do as part of some PR is update the ChangeLog, however, since this will be a major breaking change worth noting.
liftIO printGettingStartedInstructions | ||
where | ||
printGettingStartedInstructions = do | ||
putStrLn $ Term.applyStyles [Term.Green] ("Created new Wasp app in ./" ++ projectName ++ " directory!") |
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 think it is fine to use projectName
because the contract of parseProjectInfo
means it will fail if invalid, but since we use projectInfo
to create, it may also be nice to pull the project name off that struct so everything downstream uses the "validated" thing. But I don't think it warrants a change, just thinking aloud. Only reason it could matter in future is if we did any normalization for them or whatnot.
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.
Yeah, I know what you mean. I'll change it.
mkParseError ctx msg = Left $ ER.mkEvaluationError ctx $ ER.ParseError $ ER.EvaluationParseError msg | ||
stripImportPrefix importPath = stripPrefix serverPrefix importPath <|> stripPrefix clientPrefix importPath | ||
serverPrefix = "@server/" | ||
clientPrefix = "@client/" |
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.
Will we ever want to allow them to import something from @shared
?
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.
We have no such use case at the moment. All declarations have a designated runtime (server or client). I documented this restriction here.
VS Code will help them out with importing JS into Wasp after we split the ExtImport
type into ServerImport
and ClientImport
(and this code will become cleaner too). In the meantime, we're "faking" different types and pretend they exist in the docs.
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.
Sounds good 👍🏻 I will be starting a review of the docs in a bit so it will be fresh in my head :D
@@ -1,10 +1,12 @@ | |||
import { mySpecialJob } from '@wasp/jobs/mySpecialJob.js' | |||
import { sayHi } from '../shared/util.js' |
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.
This is sweet!
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.
Haha, thanks, but we already pretty much had the same thing with a smaller infrastructure 😄
maybeDotEnvServerFile <- findDotEnvServer waspDir | ||
maybeDotEnvClientFile <- findDotEnvClient waspDir |
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.
Did this issue get created BTW?
Separate client code and server code.
Fixes #522
Fixes #594
Takes a step towards implementing #710.
@Martinsos and @shayneczyzewski, feel free to start reviewing this. I've not yet updated e2e tests, example projects, or the docs. I'll do this once we settle on a structure. The current approach is:
I'd probabaly like to move the env files inside
client
andserver