kestasjk / webDiplomacy

Play Diplomacy online
http://webdiplomacy.net/
GNU Affero General Public License v3.0
179 stars 113 forks source link

Use relative paths for API #811

Closed adamlerer closed 1 year ago

adamlerer commented 2 years ago

Hi @kestasjk . With the 0.3 release, the beta UI isn't working in prod, due to using env-specified paths for the backend that weren't set to webdiplomacy.net when we compiled.

We're discussing on our end what's the best way to do this in the future. For the moment can you see if switching back to relative paths fixes the issue?

cc @utiq

kestasjk commented 2 years ago

Hi @adamlerer ,

Is it definitely not working at the moment? It seems to be working here after I added the undefined folder workaround.

I thought perhaps it was somehow detecting when I went into Web Developer tools to view the page, since it was working fine for me long after we committed but then suddenly stopped. In the error logs there were only three IPs I could see that have this issue: image

FYI users can use www.webdiplomacy.net or webdiplomacy.net , and could have authentication errors if it has the domain encoded in.

Just want to be a bit clearer on what's going wrong before applying this. I would have thought that we would get heaps of error reports and that the error logs would be full if the issue is that all requests were going to the wrong place. Was there a reason for switching to using the environment variable or was that just for dev purposes?

Rgds, Chris

utiq commented 2 years ago

Hello guys. I wouldn't suggest to go back to relative path because is going to break everything on dev environment. Also, it's not a good practice to use relative paths in production.

The fix is easy, it is just to add the production env variables when the app is built. Let me do that in the morning. Which branch should I use? This is happening in the master branch?

adamlerer commented 2 years ago

What is the "undefined folder workaround"? It has indeed started working for me when I tried it now. If it's working lets leave it alone until we can discuss the tradeoffs.

The source of the issue is that I ran npm run build without setting REACT_APP_WD_BASE_URL=https://webdiplomacy.net in my .env.X file, so this var was undefined and it was trying to access the backend through the path undefined/api.php instead of https://webdiplomacy.net/api.php.

We've gone back and forth between the env var and relative paths... I prefer relative paths because if you use env vars, then they need to be configured differently for dev (localhost) vs prod (https://webdiplomacy.net) vs other people hosting the site, and these hardcoded paths get checked in to the hardcoded js files in /beta and can't really be tested until you run in prod where you can hit that backend.

However @utiq and others have told me that relative urls are bad practice, which is probably true, but I haven't figured out why. P.S. @utiq I tried the dev environment with the relative paths and it seemed to work for me?

utiq commented 2 years ago

@kestasjk I cannot create a PR, can you add me as a collaborator so I can push the changes?

kestasjk commented 1 year ago

Right or wrong, to be honest the convention is that paths are relative and it’ll create issues where users that are logged in on www.WebDiplomacy.nethttp://www.WebDiplomacy.net won’t work if the static url doesn’t have the www. And they need to authenticate against https://webdiplomacy.net/api.php . There is a STATICSRV define() that you could use to inject a static site URL if you need to have a static URL locally for dev purposes, which is usually an empty string.

The undefined folder workaround was that commit I did that added “beta/undefined/api.php” and “beta/undefined/Ajax.php” That does seem to be working now, Im sure we would be getting lots of complaints if it wasn’t working across the board but Im only seeing positive feedback at the moment.

So I would say the way to go would be to go relative for the moment. Could we go back to doing the first commit to the staging branch, since now people are using the new UI much more heavily?

BR, Kestas

From: Adam Lerer @.> Sent: Saturday, 9 July 2022 11:25 AM To: kestasjk/webDiplomacy @.> Cc: kestasjk @.>; Mention @.> Subject: Re: [kestasjk/webDiplomacy] Use relative paths for API (PR #811)

What is the "undefined folder workaround"? It has indeed started working for me when I tried it now. If it's working lets leave it alone until we can discuss the tradeoffs.

The source of the issue is that I ran npm run build without setting REACT_APP_WD_BASE_URL=https://webdiplomacy.net in my .env.X file, so this var was undefined and it was trying to access the backend through the path undefined/api.php instead of https://webdiplomacy.net/api.php.

We've gone back and forth between the env var and relative paths... I prefer relative paths because if you use env vars, then they need to be configured differently for dev (localhost) vs prod (https://webdiplomacy.net) vs other people hosting the site, and these hardcoded paths get checked in to the hardcoded js files in /beta and can't really be tested until you run in prod where you can hit that backend.

However @utiqhttps://github.com/utiq and others have told me that relative urls are bad practice, which is probably true, but I haven't figured out why. P.S. @utiqhttps://github.com/utiq I tried the dev environment with the relative paths and it seemed to work for me?

— Reply to this email directly, view it on GitHubhttps://github.com/kestasjk/webDiplomacy/pull/811#issuecomment-1179469630, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAH2GTL7HJFQG5SJGLCFSD3VTDWJBANCNFSM53CNELRA. You are receiving this because you were mentioned.Message ID: @.**@.>>

kestasjk commented 1 year ago

Hi Cesar,

The official site sync live with GitHub, so we require a pull request that we then merge in so we can stage changes. Otherwise you would basically be pushing code straight onto the live site, and we want to have a bit of oversight.

We could give you push access to the staging branch, but for the official live site we need a pull request. The same process as Adam so he should be able to help you out.

Rgds, Kestas

Regards,


From: Cesar Gutierrez @.> Sent: Saturday, July 9, 2022 11:52:44 PM To: kestasjk/webDiplomacy @.> Cc: kestasjk @.>; Mention @.> Subject: Re: [kestasjk/webDiplomacy] Use relative paths for API (PR #811)

@kestasjkhttps://github.com/kestasjk I cannot create a PR, can you add me as a collaborator so I can push the changes?

— Reply to this email directly, view it on GitHubhttps://github.com/kestasjk/webDiplomacy/pull/811#issuecomment-1179565242, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAH2GTJLFANWCN5GWQUGXP3VTGN4ZANCNFSM53CNELRA. You are receiving this because you were mentioned.Message ID: @.***>

utiq commented 1 year ago

@kestasjk I wasn't trying to push code directly, I always create PRs. The problem is that I wasn't able to create remote branches in the repo, so I wasn't able to create a PR. How can we solve that?

adamlerer commented 1 year ago

Discussed with @utiq and we concluded that relative paths are the better option for this setup.

I've switched the PR to merge to staging for testing.

kestasjk commented 1 year ago

Hi Cesar,

Hmm, I can have a dig around to see if I can give you the ability to add branches if need be, but Adam/Bryan will target an existing branch when they do a PR. I think they set up branches locally, and then they do a PR against staging / master when they’re ready to commit their changes to the official repo.

If you ask Adam / Bryan how they are doing it hopefully they’ll be able to get you set up the same way.

Regards,


From: Cesar Gutierrez @.> Sent: Monday, July 11, 2022 9:50:15 PM To: kestasjk/webDiplomacy @.> Cc: kestasjk @.>; Mention @.> Subject: Re: [kestasjk/webDiplomacy] Use relative paths for API (PR #811)

@kestasjkhttps://github.com/kestasjk I wasn't trying to push code directly, I always create PRs. The problem is that I wasn't able to create remote branches in the repo, so I wasn't able to create a PR. How can we solve that?

— Reply to this email directly, view it on GitHubhttps://github.com/kestasjk/webDiplomacy/pull/811#issuecomment-1180434955, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAH2GTO4PVXMSD7P5U74XKLVTQRBPANCNFSM53CNELRA. You are receiving this because you were mentioned.Message ID: @.***>