-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
chore: use pnpm for builds #19752
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
base: main
Are you sure you want to change the base?
chore: use pnpm for builds #19752
Conversation
📖 Documentation deployed to pr-19752.preview.immich.app |
97f7e54
to
91ab806
Compare
2baf58c
to
b9f1b85
Compare
80c0bab
to
5367a86
Compare
91ab806
to
351266e
Compare
5367a86
to
3262c4e
Compare
351266e
to
89c84cc
Compare
200431d
to
08865cc
Compare
89c84cc
to
3ab0dbb
Compare
0d4a6fc
to
6d87af1
Compare
61efa75
to
9f1fdb3
Compare
Conflict Missing dependency gitignore Cli dockerfile Exiftool optional fix Update pnpm version update devcontainer for pnpm
9f1fdb3
to
687e99c
Compare
bb08f8a
to
e288f6b
Compare
e288f6b
to
6c81d08
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.
LGTM, but I think it should get more eyes before merge
docker/docker-compose.dev.yml
Outdated
- pnpm-store:/usr/src/app/.pnpm-store | ||
- server-node-modules:/usr/src/app/server/node_modules | ||
- github-node_modules:/usr/src/app/.github/node_modules | ||
- cli-node_modules:/usr/src/app/cli/node_modules | ||
- docs-node_modules:/usr/src/app/docs/node_modules | ||
- e2e-node_modules:/usr/src/app/e2e/node_modules | ||
- sdk-node_modules:/usr/src/app/open-api/typescript-sdk/node_modules | ||
- app-node_modules:/usr/src/app/node_modules | ||
- web-node_modules:/usr/src/app/web/node_modules | ||
- sveltekit:/usr/src/app/web/.svelte-kit | ||
- coverage:/usr/src/app/web/coverage |
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.
Why are we moving all those to named docker volumes?
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.
A few reasons:
- This is to avoid permissions issues between the view of the files inside the container vs the ones on the bind-mount on the host. If the container were to be run by a user other than the user that owns the root of the git-clone on the host, then the files would be created with the owner of the user running the container, and if that happened to be 'root' then the files can't be cleaned up with a simple
make clean
command - it would requiresudo make clean
which is a little more dangerous/annoying. - Isolation: All of dependency/package install commands are performed from within a docker container, which may be a different architecture (MacOS on arm) or libc (the web container uses alpine with musl instead of glibc) - and some of the dependent packages use native binary code, which only make sense in the architecture/libc version that is within the container it was executed inside. By using volumes, these dependencies are not created on the hosts filesystem.
- pnpm performs performs a clone, hard-link, or copy of files (in that order) from the content-addressable-store (
/buildcache/pnpm-store
) to the virtual store (.pnpm-store
). And then creates the dependency node structures innode_modules
using symlinks. Using volume mounts makes it more likely that hardlinks will be successful, increasing speed to install dependencies.
oh, and named volumes vs anonymous volumes because anonymous volumes are removed when container is removed, and containers are auto-removed at the end of every make
command.
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.
Soooo, generally a nice change but kind of unrelated to this PR?
// eslint-disable-next-line unicorn/prefer-module | ||
const basePath = dirname(__filename); | ||
const packageFile = join(basePath, '..', 'package.json'); | ||
const { version } = JSON.parse(readFileSync(packageFile, 'utf8')); |
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.
Why this change and why is the prefer module linting error 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.
I think because we used to get the version from the package.json file and that doesn't exist in the server directory anymore.
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 nevermind, I think it originally was removed, but it is still there in this version of 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.
Note: package.json still exists in each of the 8 workspace modules contained in this monorepo.
However, reading it like this:
const { version } = JSON.parse(readFileSync('./package.json', 'utf8'));
uses a relative path, which is resolved against the current-working-directory, which isn't /usr/src/app/server
any longer. The way to fix this is read the file from the absolute path of the server package.json. This is determined by using the CommonJS style __filename
global, mostly because the ESM alternative is more verbose, and -- well, the default output of the server is CommonJS anyways, not ESM, so using import.meta.url
would end up requiring even more steps to adapt back to CommonJS.
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.
What is the CWD 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.
This ends up being the WORKDIR in the various Dockerfile
stages.
For production images, this is /usr/src/app
(but server is at /usr/src/app/server
)
For server dev image, this is /usr/src/app
For web dev image, this is /usr/src/app/web
For devcontainer images, this is /workspaces/immich
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 see. Fair enough, thanks for explaining
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.
Is this an unrelated change?
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.
No - the format of the the paths of individual dependencies served from node_modules
when running in dev mode has changed, and this adapts the regexps to the pnpm's changes to these paths.
web/vite.config.ts
Outdated
@@ -60,3 +65,5 @@ export default defineConfig({ | |||
}, | |||
}, | |||
}); | |||
|
|||
export default mergeConfig(viteConfig, vitestConfig); |
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.
Why? AFAICT using the defineConfig
from vite is perfectly fine https://vitest.dev/config/#configuring-vitest
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'm not sure why (maybe because pnpm is more strict?), but I was not able to specify the 'test' property within a defineConfig
from vite
. I was able to specify test
when importing defineConfig
from vitest/config
- however, then there was an type-error on the plugins
property in the config. The one way that worked was what you see here - import both the vitest config and the vite config, then merge them. Also, the triple slash command in the docs did not help.
In fact, I looked at all the 'solutions' here: https://stackoverflow.com/questions/72146352/vitest-defineconfig-test-does-not-exist-in-type-userconfigexport and only the mergeConfig
technique (one of the alternatives in the comments) was the only one that worked for me.
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.
Update: I found another solution that just needs a new type import that is a little 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.
That looks good to me, thanks!
2975ee8
to
c302fcf
Compare
.github/workflows/fix-format.yml
Outdated
node-version-file: './server/.nvmrc' | ||
cache: 'npm' | ||
cache-dependency-path: '**/package-lock.json' | ||
node-version-file: './.github/.nvmrc' |
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.
Isn't this still wrong?
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 more carefully, this is actually formatting all of the projects, not just .github. You'd technically have to run a version of setup-node for each project. This all may be a moot point, since the node version is always the same for every project.
Would it make sense to just treat the node version as a workspace level setting, instead of per-project? Then, the nvmrc file could be moved up to the root directory of the git repo.
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 revert to just using 'server/.nvmrc' as before for now
c302fcf
to
c7ac088
Compare
eb1b85c
to
5bc6a15
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.
Nice! Excited for pnpm
docker/docker-compose.dev.yml
Outdated
pnpm-store: | ||
github-node_modules: | ||
server-node-modules: | ||
cli-node_modules: | ||
docs-node_modules: | ||
e2e-node_modules: | ||
sdk-node_modules: | ||
app-node_modules: | ||
web-node_modules: | ||
sveltekit: | ||
coverage: |
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.
Those aren't used anymore are they?
No description provided.