fix: docker setup and build of Excalidraw app

[JannikStreek]

Fixes #7464

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	Feb 2, 2024 7:16am
excalidraw	✅ Ready (Inspect)	Visit Preview	Feb 2, 2024 7:16am
excalidraw-package-example	✅ Ready (Inspect)	Visit Preview	Feb 2, 2024 7:16am
excalidraw-package-example-with-nextjs	✅ Ready (Inspect)	Visit Preview	Feb 2, 2024 7:16am

[JannikStreek]

@dwelle is there anything missing in this PR or how can I help to get it merged? Docker ist currently broken with the existing setup. I guess it would also help developers, if the docker setup is working.

[lazyWombat]

@JannikStreek I tried to run your PR but it does not work for me. I did the following steps:

1. Get your PR gh pr checkout 7502 ✅
2. Start docker compose docker compose up --build -d ✅
3. Install npm packages docker-compose exec excalidraw yarn install ✅
4. Start the app docker-compose exec excalidraw yarn start ❌

errno: -2, code: 'ENOENT', syscall: 'spawn xdg-open', path: 'xdg-open', spawnargs: [ 'http://localhost:3000/' ]

[vercel[bot]]

@JannikStreek is attempting to deploy a commit to the Excalidraw Team on Vercel.

A member of the Team first needs to authorize it.

[JannikStreek]

@JannikStreek I tried to run your PR but it does not work for me. I did the following steps:

1. Get your PR gh pr checkout 7502 ✅
2. Start docker compose docker compose up --build -d ✅
3. Install npm packages docker-compose exec excalidraw yarn install ✅
4. Start the app docker-compose exec excalidraw yarn start ❌

errno: -2, code: 'ENOENT', syscall: 'spawn xdg-open', path: 'xdg-open', spawnargs: [ 'http://localhost:3000/' ]

@lazyWombat Indeed, thanks for the feedback. I already solved this on my other working branch but took out these changes here, as they are changing the excalidraw configuration of the app. But without these changes it's not working out of the box. My last commit should make it work, can you test it again? I guess I would need someone from the core team to discuss if this change is wanted and, in case its not, if it's ok to incorporate it at least optional (setting it by some env vars).

[dwelle]

I guess I would need someone from the core team to discuss if this change is wanted and, in case its not, if it's ok to incorporate it at least optional (setting it by some env vars).

Hi! Can you explain a bit more what's going on here? I assume the idea is to use yarn start to start the dockerized app?

If so, then let's add a start:docker script. You could argue for us to add dev script for what start is doing currently, but I'm not sure what we'd want the "new" start script to be. What you're fixing here still feels very specific to docker (in prod you'd not want to be using Vite at all).

[JannikStreek]

I guess I would need someone from the core team to discuss if this change is wanted and, in case its not, if it's ok to incorporate it at least optional (setting it by some env vars).

Hi! Can you explain a bit more what's going on here? I assume the idea is to use yarn start to start the dockerized app?

If so, then let's add a start:docker script. You could argue for us to add dev script for what start is doing currently, but I'm not sure what we'd want the "new" start script to be. What you're fixing here still feels very specific to docker (in prod you'd not want to be using Vite at all).

@dwelle Sure. In essence, my goal in this PR is to fix the docker setup as it's broken and doesn't work anymore (Development + Production). I think both environments are important as many developers leverage docker to ease their setup. For the development case, yes with yarn start. You are also right that what I am fixing is absolutely docker related.

My last remark that you quoted specifically was targeted at the following commit https://github.com/excalidraw/excalidraw/pull/7502/commits/92700c7304dcaadf473f8821c3de1bb37c49e197

In this commit, I configured a host and disabled spawning a browser window as this isn't working from within docker (no browser installed). Now this changes the setup for people working without docker. As you said, adding something like yarn start:docker is also an option. I will change it according to this. 👍

[JannikStreek]

Changed the pr accordingly, its now docker-compose exec excalidraw yarn start:docker

[dwelle]

I guess I would need someone from the core team to discuss if this change is wanted and, in case its not, if it's ok to incorporate it at least optional (setting it by some env vars).

Hi! Can you explain a bit more what's going on here? I assume the idea is to use yarn start to start the dockerized app? If so, then let's add a start:docker script. You could argue for us to add dev script for what start is doing currently, but I'm not sure what we'd want the "new" start script to be. What you're fixing here still feels very specific to docker (in prod you'd not want to be using Vite at all).

@dwelle Sure. In essence, my goal in this PR is to fix the docker setup as it's broken and doesn't work anymore (Development + Production). I think both environments are important as many developers leverage docker to ease their setup. For the development case, yes with yarn start. You are also right that what I am fixing is absolutely docker related.

My last remark that you quoted specifically was targeted at the following commit 92700c7

In this commit, I configured a host and disabled spawning a browser window as this isn't working from within docker (no browser installed). Now this changes the setup for people working without docker. As you said, adding something like yarn start:docker is also an option. I will change it according to this. 👍

Yeah I was just asking specifically about the Vite config changes :p, I get the general idea of this PR — which btw we're thankful for (and sorry for the delay)!

Ok, we'll just need to figure out a way around the mts thing that Vercel complains about and we'll be set.

[JannikStreek]

I guess I would need someone from the core team to discuss if this change is wanted and, in case its not, if it's ok to incorporate it at least optional (setting it by some env vars).

Hi! Can you explain a bit more what's going on here? I assume the idea is to use yarn start to start the dockerized app? If so, then let's add a start:docker script. You could argue for us to add dev script for what start is doing currently, but I'm not sure what we'd want the "new" start script to be. What you're fixing here still feels very specific to docker (in prod you'd not want to be using Vite at all).

@dwelle Sure. In essence, my goal in this PR is to fix the docker setup as it's broken and doesn't work anymore (Development + Production). I think both environments are important as many developers leverage docker to ease their setup. For the development case, yes with yarn start. You are also right that what I am fixing is absolutely docker related. My last remark that you quoted specifically was targeted at the following commit 92700c7 In this commit, I configured a host and disabled spawning a browser window as this isn't working from within docker (no browser installed). Now this changes the setup for people working without docker. As you said, adding something like yarn start:docker is also an option. I will change it according to this. 👍

Yeah I was just asking specifically about the Vite config changes :p, I get the general idea of this PR — which btw we're thankful for (and sorry for the delay)!

Ok, we'll just need to figure out a way around the mts thing that Vercel complains about and we'll be set.

👍 no worries. I changed it to .mjs as given in the lint error. At least it still works for me locally, not sure if thats the best way to handle it. Haven't used m* modules so far.

[modem7]

Just an FYI, #7430 also covers fixing the build.

Mostly commenting so that the multiple PR's are linked so the best route forward can be merged.

[ukashazia]

Why isn't this merged yet?