the new base path that web.comm.app will proxy to needs to be added to the express router. this diff makes that change and also deduplicates logic around generating route paths.
depends on D3414
Details
Details
flow, ran the web app and everything looked fine
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- varun/eng-660 (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| server/src/server.js | ||
|---|---|---|
| 120–122 ↗ | (On Diff #10401) | Are you sure it is correct to use an array as a first parameter here? |
| 141 ↗ | (On Diff #10401) | Should we have another ws entry for commAppBaseRoutePath? |
| server/src/utils/urls.js | ||
| 44 ↗ | (On Diff #10401) | In this case we should use $ReadOnlyArray<string> or string[]. Generally, we should prefer $ReadOnlyArray, but when it is a return value, that is generated inside the function, it doesn't matter that much. If we need a mutable array, we should usually prefer type[]. We can use Array<> but it's more verbose, so I think it should be used only when necessary (so when type[] doesn't work for some reason). |
| server/src/server.js | ||
|---|---|---|
| 120–122 ↗ | (On Diff #10401) | yeah, it seems to work but it's unclear if arrays are officially supported. it's probably better to just loop through the array ourselves. fixed in latest revision |
| 141 ↗ | (On Diff #10401) | No, I think we should just proxy /{new base path}/ws to ws://localhost:3000/ws with apache. This worked fine for me locally |
| server/src/utils/urls.js | ||
| 44 ↗ | (On Diff #10401) | went with string[] |
| server/src/server.js | ||
|---|---|---|
| 120–122 ↗ | (On Diff #10401) | Ok, that makes sense. We can consider creating our own function that loops and configures the paths based on handler and path to avoid a lot of for loops that affect the readability |