-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Bringing Prisma entities to the frontend #962
Conversation
@@ -10,8 +10,9 @@ where | |||
|
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 originally rearranged the functions to go with our conventions, but it made the diff too unclear. I'll shuffle them around in a different PR.
Hey @sodic, I was just starting to take a look at this and did my usual "run it first to understand" approach. I found two things, which I will separate into two comments for easier tracking. Just wanted to raise them ASAP in case they change anything in your code/setup. Finding 1) the output appears to change each time I run, possibly indicating some underlying concurrency issue? Before:
After (clean/compile run 1):
After (clean/compile run 2):
After (clean/compile run 3):
Notice in After run 1 we get an error, After run 2 looks "normal", and After run 3 appears to run things twice. All of those runs are from a |
Finding 2) The Before:
After:
|
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! Let's see if we can maybe use ENV vars in prisma schema, if not ok let's have two files. We can add comment in client prisma schema explaining that it is a duplicate.
I approved because I think there is nothing very questionable, so once you solve all the comments you can merge.
@@ -34,3 +39,17 @@ npmVersionRange = SV.Range [SV.backwardsCompatibleWith latestLTSVersion] | |||
|
|||
prismaVersion :: SV.Version | |||
prismaVersion = SV.Version 4 5 0 | |||
|
|||
makeJsonWithEntityNameAndPrismaIdentifier :: String -> Aeson.Value | |||
makeJsonWithEntityNameAndPrismaIdentifier name = |
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.
makeJsonWithEntityNameAndPrismaIdentifier name = | |
makeJsonWithEntityNameAndPrismaIdentifier entityName = |
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 change in #982 to minimize conflicts.
concurrently | ||
(readJobMessagesAndPrintThemPrefixed chan) | ||
(DbJobs.generatePrismaClient genProjectRootDirAbs chan) | ||
readJobMessagesAndPrintThemPrefixed chan |
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.
praise: Ah yes nice!
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.
Could running two jobs concurrently with readJobMessagesAndPrintThemPrefixed
be the source of some of that output confusion? The minute it gets one job exit message it stops printing.
Also, given we got an error in the output for one of my test runs, I wonder if we should just run them sequentially to play it safe?
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 think this is only possible place where the bug could arise.
@Martinsos and I had a discussion on whether we should go with two files (schema.server.prisma
and schema.client.prisma
) or do something else (e.g.,. creating symbolic links for either the files or the generated directory, edit the file in-memory and call the prisma with that, create a temporary file and delete it, etc.)...
But now, with all the output and concurrency confusion (we'll report the same generation messages twice), I think it might be best to ditch this approach and instead:
- Generate the
.prisma/client
directory for the server - Copy it to the web app's node_modules
What do you think @shayneczyzewski (I know what @Martinsos thinks :))?
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'd be ok with doing that instead. My only concern is do we know for sure that .prisma/client
is 100% standalone and relocatable? For instance, are there any other things we have to copy over, and are there any paths in there that are relative to server stuff? If they generate identical output, I think copying is fine. 👍🏻
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 should be, but I'll jump over to their discord and make sure.
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.
Same as @shayneczyzewski @sodic , if option 3) works in all cases and environemnts, then sure! It does sound like it might be tricky when deploying though, possibly. You will know that the best.
But if we need to go instead with other options, then I would say both 1) and 2) sound good. Maybe 2) a bit better.
Summary: I don't have to much offer here I think, sounds like you can @sodic make the best choice here, just make sure it works in all scenarios. None of these options is terrible, I would say all are 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.
Argh, deploying couldn't work (at least not in the way it currently works). I'll stick with option 2) for now
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 What about the second question, do you have a preference on where we should keep the schema files?
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.
Hm not sure @sodic ! At this point, whatever you think is best, I think I will agree with.
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 what we have in the end:
- We generate a single schema file (
shema.prisma
) - We set the
client.output
field to gets its value from the environment variablePRISMA_CLIENT_OUTPUT_DIR
- We generate two Prisma clients (for both the server and the web app) by passing a different value of the
PRISMA_CLIENT_OUTPUT_DIR
env var to the same code and schema. The outputs are coming fromServer
andWeb app
respectively.
Oh wow nice job @shayneczyzewski! |
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 nice job! Overall I think the approach makes sense, I can't think how we can avoid it unless we want to manually copy things around after generating for the server. That feels hackier to me.
Aside from the two things I noted (and I put in a comment on one area that may be the concurrency thing), one last thing you will want to do is update the Dockerfile
. Inside, it has references to the old single schema:
{=# usingPrisma =}
COPY db/schema.prisma ./db/
RUN cd server && npx prisma generate --schema=../db/schema.prisma
{=/ usingPrisma =}
concurrently | ||
(readJobMessagesAndPrintThemPrefixed chan) | ||
(DbJobs.generatePrismaClient genProjectRootDirAbs chan) | ||
readJobMessagesAndPrintThemPrefixed chan |
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.
Could running two jobs concurrently with readJobMessagesAndPrintThemPrefixed
be the source of some of that output confusion? The minute it gets one job exit message it stops printing.
Also, given we got an error in the output for one of my test runs, I wonder if we should just run them sequentially to play it safe?
* Adding uninstall command * Updates docs and changelog related to the uninstall command
80bfcd8
to
e7a3386
Compare
Yes, but all TS-related doc changes are coming in a different PR. That's next on my list. |
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.
Seems the locked file issue is Mac only and exists in prior versions. We are trying to determine if it is related to some recent macOS release or something like that. So that is not in this PR and as such all looks good to me! 🚀 Nice job!
"dev", | ||
"--schema", | ||
SP.toFilePath schemaFile, | ||
"--skip-generate" |
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.
By default, Prisma generates a single Prisma client (based on the schema). Since we need two clients, I disabled this option and generated the clients explicitly in Migrate.hs
.
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 a quick review, all looking good at the top level! I did leave some minor comments, check them out.
I am not approving because I don't think I did a deep enough review to approve it. If you need approval I can do another deeper look.
readProjectTelemetryFile telemetryCacheDirPath projectHash = do | ||
fileExists <- SD.doesFileExist filePathFP | ||
if fileExists then readCacheFile else return Nothing | ||
readProjectTelemetryCache :: Path' Abs (Dir TelemetryCacheDir) -> ProjectHash -> IO (Maybe ProjectTelemetryCache) |
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.
Nit.: I think the point of having File
in the name was to indicate this function reads a file, which is now not evident any more. Maybe it is not important though. But I would consider leaving it. So you could just add File
at the end. But ok, if you like this better that is also 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.
Sure, I can change it back.
waspc/src/Wasp/Generator/Common.hs
Outdated
import qualified Wasp.SemanticVersion as SV | ||
|
||
-- | Directory where the whole web app project (client, server, ...) is generated. | ||
data ProjectRootDir | ||
|
||
class ComponentRootDir 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.
Ok, we introduced components! Maybe this is good place to put a short comment explaining what component is?
I also wonder if we should have called it AppComponent
? What do you think? Just to give it a bit more scope, Component
feels like it is missing context.
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.
Thought about it, but decided against it.
Bringing another word into the name always makes it a little more difficult to parse (i.e., the brain does not immediately know how to group the words).
Additionally, it doesn't feel like AppComponent
tells us much more than Component
. App
is a word that carries very little information in the context of our codebase (i.e., I can't think of a place where Component
is an appropriate name, while AppComponent
isn't).
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.
Place where Component is an appropriate name, while AppComponent isn't -> in the context of React/Vue/... .
Component means -> part of something. It doesn't say of what. So without App, it doesn't mean much. I would certainly push in the direction of putting App in front of it.
dbSchemaFileInProjectRootDir, | ||
prismaClientOutputDirEnvVar, | ||
databaseUrlEnvVar, | ||
DbSchemaChecksumFile, |
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.
Wow such big amount of exports indicates we might want to split this file into multiple files?
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.
If we were to split it, I think it makes sense to go with Common.hs
and Locations.hs
(or Paths.hs
). I'd like that organization better, but if I change it in Db
, I think I should do it in Server
and WebApp
as well.
This PR was already too long, so I against moving more stuff around. If you like my proposal, I can do it in a separate PR.
class DbSchemaChecksumFile f | ||
|
||
-- | This file represents the checksum of the Prisma db schema at the point | ||
-- at which we last interacted with the DB to ensure they matched. | ||
-- It is used to help warn the user of instances when they may need to migrate. | ||
data DbSchemaChecksumOnLastDbConcurrenceFile | ||
|
||
-- | This file represents the checksum of schema.prisma | ||
instance DbSchemaChecksumFile DbSchemaChecksumOnLastDbConcurrenceFile | ||
|
||
-- | This file represents the checksum of the Prisma db schema | ||
-- at the point at which `prisma generate` was last run. It is used | ||
-- to know if we need to regenerate schema.prisma during web app generation or not. | ||
data DbSchemaChecksumOnLastGenerateFile | ||
|
||
instance DbSchemaChecksumFile DbSchemaChecksumOnLastGenerateFile |
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!
|
||
generatePrismaClients :: Path' Abs (Dir ProjectRootDir) -> IO (Either String ()) | ||
generatePrismaClients projectRootDir = do | ||
generateResult <- liftA2 (>>) generatePrismaClientForServer generatePrismaClientForWebApp projectRootDir |
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.
Oh man liftA2 I have no idea what that does :D. Maybe I should learn it, or maybe this is one of those things where using cool stuff reduces readibility.
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's right there on the edge 😄
See my comment here: #962 (comment)
It's the same thing as liftM2
, but liftM2
will get deprecated (since all Monads are Applicatives).
waspc/src/Wasp/Util/IO.hs
Outdated
doesFileExist :: Path' r (File f) -> IO Bool | ||
doesFileExist = SD.doesFileExist . SP.toFilePath | ||
|
||
readFile :: Path' r (File f) -> IO String | ||
readFile = P.readFile . SP.toFilePath | ||
|
||
writeFile :: Path' r (File f) -> String -> IO () | ||
writeFile = P.writeFile . SP.toFilePath | ||
|
||
removeFile :: Path' r (File f) -> IO () | ||
removeFile = SD.removeFile . SP.toFilePath |
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, so this removes the need to do casting from SP to FP.
One thing that we kind of lost, maybe, was that before we were doing that casting a bit smarter, we were using SP.fromAbsDir
or SP.fromRelDir
and similar. I wonder if that was better. It helps you avoid mistakes where you are trying to do something with the file, but you by accident provided rel path instead of abs and it is not relative to what you wanted it to be, and you are in a problem. That is why I prefer working with abs files on disk, less space for mistake.
One option would be to add more functions here: readFileAbs
, removeFileAbs
, ... .
But that sounds cumbersome. But I would probably then try to use mostly those in the code, instead of those that allow anything.
I am not sure. 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 knew you were going to mention this, but forgot to put in a comment. Nice catch!
Anyway, I had this dilemma. Here are my thoughts:
It helps you avoid mistakes where you are trying to do something with the file, but you by accident provided rel path instead of abs and it is not relative to what you wanted it to be, and you are in a problem.
This is true, but you could still just use toFilePath
(and that's exactly what was used in half of these places). In other words, we have no way of forcing the caller to properly perform the conversions (and this way at least we know it's not a Dir
).
One option would be to add more functions here: readFileAbs, removeFileAbs, ... .
I also thought about doing this, but decided against it in favor of matching the already existing interface (from Prelude
and System.Directory
).
In the end, we want to write a file. A file can be written by specifying either its relative path or absolute path. From that point of view, I think current polymorphic signatures make the most sense.
Still, I see why it would be nice to be explicit about whether the paths are relative or absolute.
To sum up, I don't know 😄, but here are my preferences from favorite to least favorite:
- Keep these functions polymorphic and see whether it causes problem. If it does, we revisit it.
- Add
readFileAbs
andremoveFileAbs
(which would just be differently-typed aliases forreadFile
) and use those. - Revert to explicit conversions and regular
readFile :: FilePath -> 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.
True, we had no way to force it, but now there is no way to do it (if you use SP functions).
All that you said makes sense.
Having these functions that you defined makes sense in a way that they specify Standard as System and Type as File. That is already valuable, so I wouldn't drop them.
I do think it might make a lot of sense to offer their Abs
versions though, I personally would be using those quite a bit probably.
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 kept the names, but changed the signatures and added a comment explaining the reasoning.
Co-authored-by: Martin Šošić <Martinsos@users.noreply.github.com>
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.
LGTM!
Details
Partially Fixes #901 (client-side support).
Constraints:
.wasp/out/web-app/node_modules/.prisma/client/
generator.output
field inside a schema file, and resolving said path relative to the schema file.Solution
Update: Check #962 (comment) for the current approach (and the discussion behind it).
Original approach
We create two schema files in
.wasp/out/db
:schema.server.prisma
).schema.client.prisma
).Apart from the specified path, these two files are identical.
We use the server's CLI to generate both
.prisma/client
directories. We do this by running.wasp/out/server/.bin/prisma
with two different schema files. Therefore, the web app doesn't needprisma
as a dependency.We use the client schema file (
schema.client.prisma
) only for generating the.prisma/client
for the web app. The server schema file (schema.server.prisma
) is used for everything else (e.g., generating.prisma/client
for the server, checking whether the schema has been generated, checking migrations,...).PR Guide
A lot ended up happening in this PR, but there aren't as many changes as it seems. Three main ideas are at play:
StrongPath
wrappers around common file system functions (i.e.,readFile
,writeFile
,doesFileExist
andremoveFile
in Wasp.IO.Util`).FilePath
toStrongPath
and small refactoring where applicable).