per https://linear.app/comm/issue/ENG-660/make-web-app-accessible-via-commapp, we need a different JSON file for the new web app URL. this diff adds some functions to fetch the appropriate data from files and introduces a small rename.
Details
Details
- Reviewers
atul tomek - Commits
- rCOMMec738a275b15: [server] initial commit to support new app URL
ran flow
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- varun/eng-660 (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
i'm gonna have to update the buildkite script to create a new_app_url.json file
Did it for BuildKite, but you're going to have to do it for .github/workflows/eslint_flow_jest.yaml in this diff (or I guess one that's landed before this) to keep things green on GitHub
Comment Actions
It was not clear to me why do we need a new file with a url. After some time I realized that we should still support the old one, because of the native clients - but this should be explained in the summary.
There's a lot of going on in this diff:
- A new function that reads a new file is added
- We check in a lot of places if we're using a new url - should be a separate function
- We choose which facts to use
- We modify the router to serve the new path
We should split this diff into multiple ones, each having only one purpose
| server/src/responders/handlers.js | ||
|---|---|---|
| 52 ↗ | (On Diff #10284) | We have this logic (startsWith(basePath)) duplicated in a lot of places. It would be a good idea to extract it to a function, preferably the one that takes base path and returns AppUrlFacts. |
| server/src/session/cookies.js | ||
| 539 ↗ | (On Diff #10284) | Adding the flag isn't a good idea. It works ok now, but what if we wanted to introduce yet another url in the future? A much more maintainable approach could be to instead accept AppURLFacts as a parameter. |
Comment Actions
will address inline comments and split this into multiple diffs
| server/src/responders/handlers.js | ||
|---|---|---|
| 52 ↗ | (On Diff #10284) | makes sense |
| server/src/session/cookies.js | ||
| 539 ↗ | (On Diff #10284) | yeah fair enough. i was thinking we'd have at most one old and one new URL at any point in time, but i guess we could have more... |
Comment Actions
Add a new function that reads json file for commAppURLfacts, rename appURLFacts to squadCalURLFacts, add new function that returns the appropriate AppURLFacts based on the URL provided
| server/src/utils/urls.js | ||
|---|---|---|
| 34 | probably could have put this in a separate diff but it's so small that I just included it here | |
Comment Actions
Thanks for splitting the diff - it's a lot easier to review now!
| server/src/utils/urls.js | ||
|---|---|---|
| 35–37 | Just a nit: we can avoid calling getCommAppURLFacts function twice | |