Skip to content
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

build: improvements for TS & Webpack & Docker #1225

Merged
merged 33 commits into from
Mar 8, 2025

Conversation

pano9000
Copy link
Contributor

@pano9000 pano9000 commented Feb 18, 2025

Hi,

this PR (still in draft) aims to improve the build process and better utilize TS / Webpack combination

still a little bit work left to do here, but wanted to make this more visible to avoid any "duplicated work", while I work on this.

more details about the what and why and some comparison data, will be provided tomorrow.

if you try to understand my changes before that -> I recommended going through the commits chronologically, instead of looking only at the "Files changed" overview :-)

Edit:

Overview of changes

  • concentrate build output into one single folder: "dist", previously we had

    • build (from tsc),
    • app-dist (from webpack), which also "stained" the "src/public" folder with build related output
    • dist (from copy-dist), which literally copied contents of build and app-dist
  • introduced a separate tsconfig for transpiling the server side code only

    • previously tsc would transpile server side AND client side code during
    • the transpiled client side code would remain unused/useless, since webpack would transpile the code itslef and then bundle it
      • new tsconfig.build.json now only builds server side code
      • the "general" tsconfig.json still remains valid, for dev purposes
  • added build:ts and build:clean (for easily removing build folder)

  • improved copy-dist

    • simplified code overall
    • avoid copying of unnecessary and duplicate files/folders
  • updated Docker build (Dockerfile)

    • updated to reflect this new building "reality" introduced by changes above
    • make use of copy-dist instead of manual file copying -> more DRY this way
    • get rid of "hybrid" build -> now we build the full project inside Docker (which is what Docker is made for)
    • previously we did a partial build locally, copying the partially build into Docker and then doing the last remaining steps
  • updated Docker build (Github CI)

    • since we got rid of the "hybrid" buidling, we need to update the Github CI actions that use Docker

@pano9000 pano9000 force-pushed the build_improve-ts-webpack branch 2 times, most recently from 5e9a33d to 6a82a3a Compare February 23, 2025 18:02
@eliandoran
Copy link
Contributor

Now that's a lot of changes. 😮

@pano9000
Copy link
Contributor Author

hope that's not a problem ;-)
I am almost finished with this PR, just need to still figure out two things:

a) make sure swc will work with Gihub actions (currently it fails, but I have an idea why that might be)
b) make adjustments to the copy-dist.ts file, to update copying of assets (translations, stylesheets, etc.)

@pano9000
Copy link
Contributor Author

pano9000 commented Feb 24, 2025

update: I'll probably split out the swc Terser plugin work onto its own PR, so that we can at least have the improved ts/webpack building – and the build speed optimization (which would've been introduced by swc TerserPlugin) can come in later, after some additional investigation on how to handle the most elegant way:

→ it is currently giving me some issues during Github CI, due to it requiring "platform" specific packages (e.g. like @swc/core-darwin-x64) – which do not seem to be installed correctly, when we use npm ci.
Other have solved it by "removing" package-lock.json prior to npm i, but that doesn't seem like a proper "elegant" solution to me.

Potentially will also try to just install the required deps in the CI, after running npm ci – but that would not be as elegant, as I'd like – it would mean extra code per platform.

https://forum.strapi.io/t/package-error-swc-code-darwin-x64-deploying-to-strapi-cloud/37922
swc-project/swc#2898
https://stackoverflow.com/a/77721414
https://nextjs.org/docs/messages/failed-loading-swc

=> will update the PR accordingly this evening

@pano9000 pano9000 force-pushed the build_improve-ts-webpack branch 3 times, most recently from 5915250 to 2fdb9cb Compare February 27, 2025 08:44
@pano9000
Copy link
Contributor Author

pano9000 commented Feb 27, 2025

I'd say I am done with this PR now, just need to do a quick test on a Windows machine as well, to make sure the copy-dist script does not behave a bit unexpectedly and I'll then rebase onto latest develop to avoid merge conflicts.

edit: confirmed on Win11 -> copy-dist works exactly as on my Linux machine :-)

Quick first stats from these changes:

develop branch

Dist Folder size:
    du -sch dist/
    282M    dist/

Number of files:
    find ./dist/ | wc -l
    6321

vs

this PR


    
Dist Folder size:
    du -sch dist/
    172M    dist/

Number of files:    
    find ./dist/ | wc -l
    3781

some more details to follow on what exactly changed later this evening

edit:
Docker building needs to be updated as well, it seem - the CI is failing, but I know where and why it goes wrong -- will fix

@pano9000 pano9000 force-pushed the build_improve-ts-webpack branch from 2fdb9cb to e810715 Compare March 3, 2025 08:37
@pano9000
Copy link
Contributor Author

pano9000 commented Mar 3, 2025

update on the docker build:
I have a working version locally, but need to clean it up a bit, before pushing – the benefit here is that the build process now actually happens inside Docker and not the previous weird "hybrid" way, where part of it was built "locally" and then copied inside Docker for further steps.

=> also I do realize that this PR is getting bigger and bigger, but the issue is: The build process was so messy, you start pulling one thread, and you end up needing to take care of every other little bit as well :-D

@pano9000 pano9000 changed the title build: improvements for TS & Webpack build: improvements for TS & Webpack & Docker Mar 3, 2025
@pano9000 pano9000 force-pushed the build_improve-ts-webpack branch 2 times, most recently from 9e3a20d to fb49467 Compare March 5, 2025 08:13
@pano9000
Copy link
Contributor Author

pano9000 commented Mar 5, 2025

another update on the status here:

Docker build is working now and looks a lot cleaner (and even a tiny bit smaller in size) than before.

Couple of things left to do:

  • do some tests with the github CI, to make sure I didn't break anything accidentally :-)

that will probably happen tomorrow and then I can finally open this up for reviews.

I will then also hopefully have a detailed list of changes and comparisons

Copy link
Contributor Author

@pano9000 pano9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick comment on this change:

the installation of these dependencies was originally added 7 years ago:
9cb9ea6

but I don't see any comment as to why these where added in the first place.
Not sure if 7 years ago they actually were needed for some rason, but currently they are definitely not, as I've confirmed by building the project without these :-)

@pano9000
Copy link
Contributor Author

pano9000 commented Mar 5, 2025

Did run a test with a slightly modified main-docker.yml workflow (where I removed the "Login" actions, to not fail the workflow, before it even begins building docker):
https://github.com/pano9000/TriliumNextNotes/actions/runs/13681461722/job/38255065185

CI builds :-)
it finally (of course) fails at pushing the images to the registry though

will provide more details and what exactly I did tomorrow, but marking this one as ready for review already, for everyone to check

@pano9000 pano9000 marked this pull request as ready for review March 5, 2025 17:41
@pano9000 pano9000 force-pushed the build_improve-ts-webpack branch 2 times, most recently from 6a7927a to d2b86df Compare March 6, 2025 21:33
@pano9000
Copy link
Contributor Author

pano9000 commented Mar 6, 2025

@eliandoran
PR is ready, overview of changes has been detailed in the first comment as well :-)

Looking forward to your review on this :-)

(afterwards I can finally start looking into Electron build improvements as well)

pano9000 added 6 commits March 7, 2025 23:14
this prevents tsc from unnecessarily transpiling the frontend part as well:
previously it was transpiled by tsc, but the files got discarded and replaced by the files built by webpack.

speeds up tsc command a bit as well:
from 14 seconds to ~8 secs
output the bundled files directly in the build folder
a) keeps the src folder clean from build output
b) it saves us some "manual" copying work
as per https://webpack.js.org/configuration/devtool/#production

serving the `source-map` file to "normal" users seems to be not recommended, so instead let's go with `nosources-source-map`:

a) this drastically reduces app-dist folder size from 20MB down to 8.7MB
b) it still allows for stack traces
not sure why, but seems like it doesn't like `[jt]s` – which causes it to skip certain .d.ts files, making tsc fail
since we build into the build folder -> we should also clean the folder before building as well

also it makes sense to run tsc first, as it runs faster, so if there's any TS errors, we will have a faster failing build
pano9000 added 26 commits March 7, 2025 23:14
these folder are already "excluded" implicitly, since we only include "./src" folder
webpack bundling already ran before this script, so there is no need to copy this file over
stop the build folder from being copied into the dist/src subfolder
→ there is no sense in doing that
→ the contents of the build folder are corretly copied previously already (see line 26ff)
we have the bundled "app-dist" already in the "dist", copying over the original unbundled "app" folder serves no benefit here
the "srcDirsToCopy" block is useless now, we can just use the previous dirsToCopy to achieve the exact same thing
… copying job

there's no benefit in having them split up like before
there's no need to read the folder structure and then copy each single file in a loop

=> just copy the whole folder and be done with it :-)
there's no benefit from stripping "node_modules/" from the string, to later add it again using the `DEST_DIR_NODE_MODULES` constant

=> just copy directly into the `DEST_DIR` folder and preserver the `node_modules` part in the path
since this is a "standalone" script we are running and no other JS scritps are running "in the background", there's no real benefit for async here.
since we don't export this anywhere, might as well just call the steps directly
TS and Webpack build into the dist folder directly now
this Dockerfile is aimed at production builds, i.e. trying to keep size as small as possible at the cost of "rebuild speed", due to missed docker cache opportunities.

Build Stage:
* do the complete build inside docker as oposed to the previous "hybrid", where tsc was run locally and the output got copied into the Docker build stage → you can now build this with Docker, without having to install the whole node/TS env locally

* build into a "build" subfolder, for easier clean up during build stage

* get rid of now unnecessary extra file/asset handling, as this is now handled by `npm run build:prepare-dist`

* no `npm prune` needed here, as we delete the whole build folder anyways in the last build step

Runtime stage:
* move the "electron" dep removal from the builder stage to the runtime stage, before installing the dependencies

* move to `npm ci` for reproducible installations – but only installing runtime deps here

* get rid of now unnecessary copying commands from the builder stage, as everything is now neatly available in "/usr/src/app"
same changes as for the "non-alpine" Dockerfile previously commited, but adapted to Alpine.

this Dockerfile is aimed at production builds, i.e. trying to keep size as small as possible at the cost of "rebuild speed", due to missed docker cache opportunities.

Build Stage:
* do the complete build inside docker as oposed to the previous "hybrid", where tsc was run locally and the output got copied into the Docker build stage → you can now build this with Docker, without having to install the whole node/TS env locally

* build into a "build" subfolder, for easier clean up during build stage

* get rid of now unnecessary extra file/asset handling, as this is now handled by `npm run build:prepare-dist`

* no `npm prune` needed here, as we delete the whole build folder anyways in the last build step

Runtime stage:
* move the "electron" dep removal from the builder stage to the runtime stage, before installing the dependencies

* move to `npm ci` for reproducible installations – but only installing runtime deps here

* get rid of now unnecessary copying commands from the builder stage, as everything is now neatly available in "/usr/src/app"
this is now handled fully inside Docker.

exception for "test_docker" job in "main-docker"
-> it seems that one needs to be there still, since it runs Playwright tests from outside the container
…es fodler from dist folder

added a TODO as well, to get rid of this strange step here at some point
@pano9000 pano9000 force-pushed the build_improve-ts-webpack branch from d2b86df to 70e227f Compare March 7, 2025 22:14
@eliandoran eliandoran merged commit acedb0e into develop Mar 8, 2025
3 of 4 checks passed
@eliandoran eliandoran deleted the build_improve-ts-webpack branch March 8, 2025 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants