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

🚀 Feature: Configurable ports and database through env #144

Closed
szlend opened this issue Apr 17, 2023 · 23 comments
Closed

🚀 Feature: Configurable ports and database through env #144

szlend opened this issue Apr 17, 2023 · 23 comments
Labels
feature New feature or request

Comments

@szlend
Copy link

szlend commented Apr 17, 2023

🔖 Feature description

The ports and database url could be configurable through environmental variables. I think at minimum you would want:

Frontend:

  • PORT=3000
  • API_URL=http://localhost:8080

Backend:

  • PORT=8080
  • DATABASE_URL=file:../data/pingvin-share.db?connection_limit=1

Right now to make this packagable for NixOS I need to apply the following patches:

Backend:

substituteInPlace src/main.ts \
  --replace 'await app.listen(8080)' 'await app.listen(+process.env.PORT)'

substituteInPlace src/prisma/prisma.service.ts \
  --replace '"file:../data/pingvin-share.db?connection_limit=1"' 'process.env.DATABASE_URL'

substituteInPlace prisma/schema.prisma \
  --replace '"file:../data/pingvin-share.db"' 'env("DATABASE_URL")' 

Frontend:

substituteInPlace src/pages/_app.tsx \
  --replace '`http://localhost:8080' '`''${process.env.API_URL}'

substituteInPlace src/middleware.ts 'src/pages/api/[...all].tsx' \
  --replace '"http://localhost:8080' 'process.env.API_URL + "'

🎤 Pitch

Installing pingivin-share on a shared system (outside of containers, e.g. NixOS) is currently difficult because the ports are hardcoded and can easily clash with other applications. The database is also hardcoded to a path relative to prisma, which makes it impossible to ship the application into a non-writable prefix (e.g. /nix/store or /usr).

@szlend szlend added the feature New feature or request label Apr 17, 2023
@stonith404
Copy link
Owner

Good point. This shouldn't be much work but currently my laptop is in repair so it could take some time until I can implement this.

When you're finished I would love if you add the NixOS installation instructions to the README :)

@stonith404
Copy link
Owner

I'm currently working on this. I think we don't need API_URL as we just could use http://localhost:${FRONTEND_PORT}. Do you think this is sufficient?

@stonith404
Copy link
Owner

Do you start the frontend with npm run start? Because it isn't possible to specify the port in the code, only in the command like this: npm run start -p $FRONTEND_PORT. Would this approach work with your installation?

@szlend
Copy link
Author

szlend commented Apr 24, 2023

I'm currently working on this. I think we don't need API_URL as we just could use http://localhost:${FRONTEND_PORT}. Do you think this is sufficient?

Not strictly necessary, but I think it’s worth having the option, say you want to sandbox each service into its own network namespace. It’s a bit of a niche use-case, but it’s definitely something I’ve done in the past.

@szlend
Copy link
Author

szlend commented Apr 24, 2023

Do you start the frontend with npm run start? Because it isn't possible to specify the port in the code, only in the command like this: npm run start -p $FRONTEND_PORT. Would this approach work with your installation?

I ship it without npm and start nodejs directly. I'm trying to deploy a minimal package, so shipping a package manager seemed unnecessary.

@szlend
Copy link
Author

szlend commented Apr 24, 2023

Can't verify right now but I think the fronted server already supports the 'PORT' env var.

@stonith404
Copy link
Owner

Yes the frontend supports the PORT env variable. The backend port should have another env variable for the port as it would clash with the PORT env variable of the frontend. Or I'm wrong?
So as a summary I would add these variables:

  • PORT - frontend port
  • BACKEND_PORT - backend port
  • BACKEND_URL - backend url
  • DATABASE_URL- database url

Is this correct?

@szlend
Copy link
Author

szlend commented Apr 24, 2023

Yes the frontend supports the PORT env variable. The backend port should have another env variable for the port as it would clash with the PORT env variable of the frontend. Or I'm wrong?

So as a summary I would add these variables:

  • PORT - frontend port

  • BACKEND_PORT - backend port

  • BACKEND_URL - backend url

  • DATABASE_URL- database url

Is this correct?

Yeah I think that's fine. Personally I don't mind the env var name clash as I run them in systemd where I can configure env vars per service. But I imagine it's useful in cases where you don't have that sort of isolation.

@stonith404
Copy link
Owner

Added in e5071cb. Let me know if this fits your needs :)

Backend

  • PORT
  • DATABASE_URL

Frontend

  • PORT
  • API_URL

@szlend
Copy link
Author

szlend commented Apr 26, 2023

@stonith404 Thank you! Though I just tried the new version and it seems like the database path is still hardcoded in schema.prisma. In my setup I replaced "file:../data/pingvin-share.db" with env("DATABASE_URL"). It doesn't seem like it's possible to set a default unfortunately: prisma/prisma#222

@stonith404
Copy link
Owner

@szlend Oh yeah, I've forgot that and that's actually I problem. Currently I'm not sure how to solve this. I really don't have a knowledge about creating packages for Linux but do you think it matters where the database file is stored? If yes, we would have to create an env variable for the path where the uploaded files are stored, or I'm wrong? Because the database file is stored inside the same folder as the uploads.

@szlend
Copy link
Author

szlend commented Apr 26, 2023

Actually, I think it could be worked around by placing an .env file in the prisma directory, which could serve as a default for DATABASE_URL. I can't test right now, but from what I could gather, system env vars should still have precedence over the .env file. This is kinda janky though.

do you think it matters where the database file is stored?

From what I remember, the database file is placed relative to the prisma schema directory. At least when running migrations using the prisma client. If you package the app into a non-writable system path, that's a problem yeah.

Because the database file is stored inside the same folder as the uploads.

The uploads folder is actually relative to the working directory which is really easy to change. You just execute the server from whatever path you want as the root, by cd-ing into it. Though I guess this probably wouldn't work through npm. Though ideally, you would be able to configure UPLOAD_PATH as well.

@stonith404
Copy link
Owner

Great idea! Creating a .env file inside the prisma folder works and the system env variables will be prioritized. Although this isn't a really beautiful solution, this would be a valid workaround. What do you think?

@szlend
Copy link
Author

szlend commented Apr 26, 2023

Great idea! Creating a .env file inside the prisma folder works and the system env variables will be prioritized. Although this isn't a really beautiful solution, this would be a valid workaround. What do you think?

That's great! Yeah it's kinda janky but I think it's worth the cost.

What do you think about also making the upload dir configurable? Currently I symlink it to another drive, but it would be nice if I could just configure it as well.

@stonith404
Copy link
Owner

I've just fixed the prisma issue and added an env variable that allows to specify the data directory. It isn't merged yet because it would be nice if you could check it out first if this works for you. The newest changes are in the configureable-database-url branch.

Here are the added env variables again:
Backend

  • PORT
  • DATABASE_URL
  • DATA_DIRECTORY

Frontend

  • PORT
  • API_URL

@szlend
Copy link
Author

szlend commented Apr 28, 2023

Awesome, I tested the latest changes and everything seems to be working fine! Gonna try and package this for nix when I get the chance.

Edit: Actually, let me check one more thing. It started successfully but the frontend is not responding.

@szlend
Copy link
Author

szlend commented Apr 28, 2023

This part doesn't work for me:

await fetch(`${request.nextUrl.origin}/api/configs`)
Error [TypeError]: fetch failed

Shouldn't this just resolve to API_URL? It's executed on the server-side.

@stonith404
Copy link
Owner

That's strange I can't reproduce this. The problem is we can't use the getConfig() function in a middleware so we have to use request.nextUrl.origin as a workaround. Would you mind to replace

const config = await (
      await fetch(`${request.nextUrl.origin}/api/configs`)
    ).json();

with

  console.log(request.nextUrl);
  let config: any;
  try {
    // Get config from backend
    config = await (
      await fetch(`${request.nextUrl.origin}/api/configs`)
    ).json();
  } catch (e) {
    console.log(e);
  }

so we can get a better error message?

@szlend
Copy link
Author

szlend commented Apr 30, 2023

Two issues:

  • On the backend, the port needs to be cast to an integer: await app.listen(+process.env.PORT)
  • On the frontend, the next.config.js file is evaluated at build time, not at runtime. So the apiURL will always be set to :8080. You can verify that by inspecting the compiled server.js.

Nit:

  • request.nextUrl.origin works but it points to the frontend which then proxies it to the backend. In production you'd likely want nginx (or whatever http entrypoint) route that. Not a big issue though, I see why it's convenient.

@stonith404
Copy link
Owner

I fixed the casting.

Additionally I've debugged the error a bit and I found out that it it takes the runtime env variables with npm run start (npx next start) correctly. I really don't have a clue why this works but node .next/standalone/server.js not. Would it be possible that you use the npx next start command?

Sure it would be nice if the hoster can setup a reverse proxy by his own but it gets really complicated if I want to support the Docker version too. I think for the most of the users it's fine to use the current implemented reverse proxy. Do you think this is enough for the moment?

@szlend
Copy link
Author

szlend commented Apr 30, 2023

Would it be possible that you use the npx next start command?

I would prefer avoiding packaging unnecessary dependencies. But yeah, I can debug this myself.

Do you think this is enough for the moment?

Yeah, no worries.

Thanks!

@stonith404
Copy link
Owner

Okay :) Let me know if you need anything

@stonith404
Copy link
Owner

Added in v0.15.0. Feel free to create another issue, if you come across any other issues :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants