Paths

Table of Contentst

-
server/src/
-
src/
-
server.js
-
utils/
1/2
urls.js

[server] add helper function to generate all route paths
ClosedPublic
Actions

Authored by varun on Mar 15 2022, 11:20 AM.

Details

Reviewers

atul
tomek
ashoat

Commits

rCOMMd81b5d932413: [server] add helper function to generate all route paths

Summary

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

Test Plan

flow, ran the web app and everything looked fine

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

varun created this revision.Mar 15 2022, 11:20 AM

Herald added subscribers: • benschac, • karol, • adrian, ashoat. · View Herald TranscriptMar 15 2022, 11:20 AM

varun edited the summary of this revision. (Show Details)Mar 15 2022, 11:21 AM

varun added a parent revision: D3414: use landing base route path to construct endpoints.

Harbormaster completed remote builds in B7390: Diff 10401.Mar 15 2022, 11:25 AM

varun requested review of this revision.Mar 15 2022, 11:25 AM

tomek requested changes to this revision.Mar 16 2022, 6:48 AM

tomek added inline comments.

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).

This revision now requires changes to proceed.Mar 16 2022, 6:48 AM

address feedback

varun added inline comments.Mar 21 2022, 12:10 PM

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[]

Harbormaster completed remote builds in B7511: Diff 10575.Mar 21 2022, 12:12 PM

tomek accepted this revision.Mar 22 2022, 4:43 AM

tomek added a reviewer: ashoat.

tomek added inline comments.

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

ashoat accepted this revision.Mar 22 2022, 7:29 AM

This revision is now accepted and ready to land.Mar 22 2022, 7:29 AM

Closed by commit rCOMMd81b5d932413: [server] add helper function to generate all route paths (authored by varun). · Explain WhyMar 22 2022, 11:17 AM

This revision was automatically updated to reflect the committed changes.

varun added a commit: rCOMMd81b5d932413: [server] add helper function to generate all route paths.

ashoat added a reverting change: D3549: Revert "[server] add helper function to generate all route paths".Mar 29 2022, 3:23 PM

ashoat added a reverting change: rCOMM40eba4f23764: Revert "[server] add helper function to generate all route paths".Mar 30 2022, 11:15 AM

Revision Contents
Changeset List

Path

Size

server/

src/

server.js

97 lines

utils/

urls.js

15 lines

Diff 10599

View Options

server/src/server.js

	Show All 21 Lines
	a {			a {
	text-decoration: none;			text-decoration: none;
	color: #2a5db0;			color: #2a5db0;
	}			}
	img,			img,
	iframe {			iframe {
	display: block;			display: block;
	}			}
	input[type='text'],			input[type='text'],
	input[type='password'],			input[type='password'],
	textarea {			textarea {
	atulUnsubmitted Not Done Inline Actions Moving the global input styling and moving it to the input component. Then why are we keeping styles like `border` and `border-radius`? Don't these styles also affect the `<textarea>` tag? Are you sure we want to be removing these styles for that as well? Or should we break this into two separate selectors (one for `input` and another for `textarea`)? atul: > Moving the global input styling and moving it to the input component. Then why are we…
	benschacAuthorUnsubmitted Done Inline Actions Yeah, I should break these styles out a bit more. I don't think they really do anything. But you make a good point that this will also effect `textarea` elements. benschac: Yeah, I should break these styles out a bit more. I don't think they really do anything. But…
	-webkit-appearance: none;
	-moz-appearance: none;
	-webkit-border-radius: 0;
	border: 1px solid #dddddd;			border: 1px solid #dddddd;
	border-radius: 1px;			border-radius: 1px;
	font-family: var(--font-stack);			font-family: var(--font-stack);
	}			}
	:global(#react-root) {			:global(#react-root) {
	display: flex;			display: flex;
	flex-direction: column;			flex-direction: column;
	height: 100%;			height: 100%;
	▲ Show 20 Lines • Show All 243 Lines • Show Last 20 Lines