Build TS in Docker, not when starting app #1008
Conversation
| @@ -47,6 +47,7 @@ WORKDIR /app | |||
| COPY --from=server-builder /app/server/node_modules ./server/node_modules | |||
| COPY server/ ./server/ | |||
| COPY db/ ./db/ | |||
| RUN cd server && npm run build | |||
There was a problem hiding this comment.
I couldn't run this in server-builder since I need the source code, which is copied over here. I didn't want to fundamentally restructure this Dockerfile, and instead do the smallest change that will work, since we will rethink this process in the near future.
There was a problem hiding this comment.
I don't quite understand this!
So we need to build a TS project. Why not build it in server-builder? I guess you would need to copy some extra files then, but I guess that wouldn't be too hard? I don't think this is too bad either, it is probably a small impact, but I am curious.
There was a problem hiding this comment.
There is an added benefit if we did it in the server-builder step, we could copy only the node_modules and dist folder to the next step (avoiding copying over the src and everything else).
But we should still separate the copying of src and package*.json files to keep the layer caching benefits.
FROM base AS server-builder
...
COPY server/package*.json ./server/
RUN cd server && npm install
...
COPY server/ ./server/
RUN cd server && npm run build
FROM base AS server-production
...
COPY --from=server-builder /app/server/node_modules ./server/node_modules
COPY --from=server-builder /app/server/dist ./server/distThere was a problem hiding this comment.
Ok ok, you all convinced me, I will just refactor it a bit to do all the building where it probably should. The 1 line fix was just so appealing though :D
shayneczyzewski
changed the title
| @@ -47,6 +47,7 @@ WORKDIR /app | |||
| COPY --from=server-builder /app/server/node_modules ./server/node_modules | |||
| COPY server/ ./server/ | |||
| COPY db/ ./db/ | |||
| RUN cd server && npm run build | |||
There was a problem hiding this comment.
I don't quite understand this!
So we need to build a TS project. Why not build it in server-builder? I guess you would need to copy some extra files then, but I guess that wouldn't be too hard? I don't think this is too bad either, it is probably a small impact, but I am curious.
There was a problem hiding this comment.
Ok @Martinsos and @infomiho, made your updates (plus one small other cleanup). Feel free to do a final check if you'd like. Thanks!
| {=# usingServerPatches =} | ||
| COPY server/patches ./server/patches | ||
| {=/ usingServerPatches =} | ||
| COPY server/package*.json ./server/ |
There was a problem hiding this comment.
We no longer have to selectively, or conditionally in the case of patches, copy a subset of the server dir over. Just copy it all, since we need the source later anyways for TS compilation.
| COPY --from=server-builder /app/server/dist ./server/dist | ||
| COPY --from=server-builder /app/server/package*.json ./server/ |
There was a problem hiding this comment.
In addition to the dist dir, we also need the package.json files to run the npm run command.
| RUN cd server && npm install | ||
| {=# usingPrisma =} | ||
| COPY db/schema.prisma ./db/ | ||
| RUN cd server && {= serverPrismaClientOutputDirEnv =} npx prisma generate --schema='{= dbSchemaFileFromServerDir =}' | ||
| {=/ usingPrisma =} | ||
| # Compile TS (depends on Prisma, if in use) |
There was a problem hiding this comment.
Nit: This comment feels a bit off
The fact that we are compiling TS here, it's not really important, we are building the server in general. If I understood the comment correctly, you wanted to convey the idea "we need to put this AFTER the prisma generate bit". Maybe write it like that 🤷
There was a problem hiding this comment.
Good point, I can drop the compile/build note and just say we need to do this after Prisma entities were generated (which was the main point, as you noted). 👍🏻 Will update comment shortly
Description
This PR moves the TS build process (
npx tsc) into the Dockerfile, thus happening at build time and not when the app starts. This should reduce the memory requirements for running the app.In the future, we will fix how production assets are built and shipped here: #897
Select what type of change this PR introduces:
Update Waspc ChangeLog and version if needed
If you did a bug fix, new feature, or breaking change, that affects waspc, make sure you satisfy the following: