Swap wouter to preact-router #6

aduth · 2021-08-25T18:13:10Z

Why: Better support for querystring-based routing

**Why**: Better support for querystring-based routing

aduth · 2021-08-25T18:16:48Z

src/report-filter-controls.js

+    const formData = /** @type {string[][]} */ (Array.from(new FormData(form)));
+    route(`${path}?${new URLSearchParams(formData).toString()}`);


Interesting case of: it works, mostly, with just new URLSearchParams(new FormData(form)), but TypeScript is extra strict about the possibility of the form including file inputs not being compatible as search parameters 🤷

microsoft/TypeScript#30584

zachmargolis

LGTM! added #6 to take advantage of these changes

* Remove unused effect dependencies * Update src/report-filter-controls.js * Simplify agency parsing now that formatting is in update() Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

aduth · 2021-08-25T20:52:55Z

Just noticed the router updates and drops the base path in Federalist. Did that happen before as well?

Edit: Related: preactjs/preact-router#279

zachmargolis · 2021-08-25T20:55:00Z

Just noticed the router updates and drops the base path in Federalist. Did that happen before as well?

The old form posts kept the path: https://federalist-14721e0a-0772-4dcd-b62f-ee9d20dcc5fd.app.cloud.gov/daily-auths-report?start=2021-08-15&finish=2021-08-22&ial=2&agency=

but also they route to a real page that 404s, so maybe we need to add some HTTP meta redirects to catch those?

aduth · 2021-08-25T20:56:16Z

The problem is it's not keeping the base path, e.g. /preview/18f/identity-reporting/aduth-preact-router/

The wrapper workaround mentioned in the above issue could work.

zachmargolis · 2021-08-25T20:58:06Z

The wrapper workaround mentioned in the above issue could work.

Ah ok, so can we update vite to detect the BASEURL and conditionally embed a <base> tag?

- Treat it as null so we don't get duplicate paths like "///"

- Don't double-add BASE_PATH - Remove leading and trailing slashes from parts so they don't double up

To allow otherProps to work like a link

aduth · 2021-08-26T13:14:34Z

It hadn't occurred to me that Federalist doesn't appear to have great single-page app support out of the box as far as allowing subpaths to match back to the _site/index.html.

Couple thoughts:

Abuse 404 page customization to show or redirect to index.html. Status code will be 404 though, which is odd, and unfortunately not customizable by the looks of it.
Use hash-based routing instead. Maybe this lets us avoid the need for wrappers altogether.
Maybe we could copy _site/index.html to [path]/index.html for each route in the Router ? 🤷

zachmargolis · 2021-08-26T15:13:31Z

Couple thoughts:

Abuse 404 page customization to show or redirect to index.html. Status code will be 404 though, which is odd, and unfortunately not customizable by the looks of it.

Use hash-based routing instead. Maybe this lets us avoid the need for wrappers altogether.

Maybe we could copy _site/index.html to [path]/index.html for each route in the Router ? 🤷

If I had to rank those options, I'd go in this order:

copy _site/index.html
- Could we use symlinks?
- Could we write a lint to help us enforce that <Route path=...> corresponds to a file?
hash-based routing
do the 404 trick

WDYT?

aduth · 2021-08-26T15:33:19Z

With the copy option, I was imagining a post-build script where we'd auto-generate these by "rendering" the routes in-memory, getting the full list of paths, then cp/ln-ing each using the base index.html. It's a bit more involved than just listing out the pages manually, but less error-prone as well.

zachmargolis · 2021-08-26T15:34:37Z

With the copy option, I was imagining a post-build script where we'd auto-generate these by "rendering" the routes in-memory, getting the full list of paths, then cp/ln-ing each using the base index.html. It's a bit more involved than just listing out the pages manually, but less error-prone as well.

🚢 !!

fix some typos

zachmargolis · 2021-08-26T16:20:35Z

bin/generate-routes.js

@@ -0,0 +1,11 @@
+import { mkdir, copyFile } from "fs/promises";
+import { join } from "path";


wait is there a version of this in the browser? could we simplify a ton of logic in getFullPath?

Not in the browser, no. This is run in Node as part of the build.

😭 too bad

zachmargolis · 2021-08-26T16:23:06Z

src/routes/report.js

+function ReportRoute() {
+  return html`<${ReportFilterControls}><${DailyAuthsReport} /><//>`;
+}


This seems to me like something we could inline in the ROUTES constant?

This seems to me like something we could inline in the ROUTES constant?

Yeah, possibly. I feel like what we're currently treating as top-level "page" components for the report page is something we'd want to have some organization of, hence the dedicated directory and component, vs. a sprawling directory of every sort of component. So for me I'd see this moving more in the direction of absorbing most of DailyAuthsReport here I guess.

I could imagine an alternative where we keep things more as they are, and have a single routes.js with our constant and (maybe) "Route" component†.

† The reason I had to move this out of main.js is because ESBuild doesn't know how to handle the CSS import statement (or does it? 🤔 either way, saw errors).

Federalist drops querystrings in un-suffixed path redirects

zachmargolis · 2021-08-26T19:02:39Z

src/router.js

+    return (
+      "/" +
+      [basePath, path]
+        .map((p) => p.replace(/^\/|\/$/g, ""))


now that we added trailing slashes in the router, we probably want to change this to drop trailing slashes only on basePath, and preserve trailing slashes for path

Swap wouter to preact-router

6f6b050

**Why**: Better support for querystring-based routing

aduth requested a review from zachmargolis August 25, 2021 18:13

aduth commented Aug 25, 2021

View reviewed changes

zachmargolis mentioned this pull request Aug 25, 2021

Page live-updates to sync with form #7

Merged

zachmargolis approved these changes Aug 25, 2021

View reviewed changes

Page live-updates to sync with form (#7)

e6ae015

* Remove unused effect dependencies * Update src/report-filter-controls.js * Simplify agency parsing now that formatting is in update() Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

aduth and others added 12 commits August 25, 2021 17:08

Try router wrapper

ca28672

Handle when BASE_URL is "/"

8b89934

- Treat it as null so we don't get duplicate paths like "///"

Make a pass at adding types

0b7ff5c

Add sourcemaps to debug in federalist

aa54089

Handle getFullPath("/") so it doesn't return "///"

505d284

Update getFullPath

37edd20

- Don't double-add BASE_PATH - Remove leading and trailing slashes from parts so they don't double up

Stub import meta for tests

f6bc3a3

Fix lint errors

b877337

Fix multi-slash path

d8de33c

Include Mocha types

8e6c04a

Use HTMLAttributes for Link props

9d8409d

To allow otherProps to work like a link

Restore base path detection in getFullPath

e180f4a

zachmargolis added 2 commits August 26, 2021 08:29

Add tests for getFullPath

3f9ba0c

eslint semicolon rule

7bbf463

zachmargolis and others added 2 commits August 26, 2021 08:35

Update router.test.js

7093cd1

fix some typos

Define routes as constant

3707ec0

aduth added 2 commits August 26, 2021 12:11

Treat import meta as optional

338edc7

Copy index.html for every route

ca4db34

zachmargolis reviewed Aug 26, 2021

View reviewed changes

Fix BASE_URL undefined

aa7fa54

zachmargolis reviewed Aug 26, 2021

View reviewed changes

aduth and others added 4 commits August 26, 2021 12:33

drop casting (for now)

895f6d2

pass props to filter controls

6546072

:shushing-face: typescript

b87821a

Add trailing slash for routes

f212002

Federalist drops querystrings in un-suffixed path redirects

zachmargolis reviewed Aug 26, 2021

View reviewed changes

aduth added 2 commits August 26, 2021 15:11

try simpler getFullPath

03a6474

Add test case for preserving trailing slash

5cefcc6

aduth merged commit f4b183f into main Aug 26, 2021

aduth deleted the aduth-preact-router branch August 26, 2021 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swap wouter to preact-router #6

Swap wouter to preact-router #6

aduth commented Aug 25, 2021

aduth Aug 25, 2021

zachmargolis left a comment

aduth commented Aug 25, 2021 •

edited

Loading

zachmargolis commented Aug 25, 2021

aduth commented Aug 25, 2021

zachmargolis commented Aug 25, 2021

aduth commented Aug 26, 2021 •

edited

Loading

zachmargolis commented Aug 26, 2021

aduth commented Aug 26, 2021

zachmargolis commented Aug 26, 2021

zachmargolis Aug 26, 2021

aduth Aug 26, 2021

zachmargolis Aug 26, 2021

zachmargolis Aug 26, 2021

aduth Aug 26, 2021

zachmargolis Aug 26, 2021

		const formData = /** @type {string[][]} */ (Array.from(new FormData(form)));
		route(`${path}?${new URLSearchParams(formData).toString()}`);

		@@ -0,0 +1,11 @@
		import { mkdir, copyFile } from "fs/promises";
		import { join } from "path";

Swap wouter to preact-router #6

Swap wouter to preact-router #6

Conversation

aduth commented Aug 25, 2021

aduth Aug 25, 2021

Choose a reason for hiding this comment

zachmargolis left a comment

Choose a reason for hiding this comment

aduth commented Aug 25, 2021 • edited Loading

zachmargolis commented Aug 25, 2021

aduth commented Aug 25, 2021

zachmargolis commented Aug 25, 2021

aduth commented Aug 26, 2021 • edited Loading

zachmargolis commented Aug 26, 2021

aduth commented Aug 26, 2021

zachmargolis commented Aug 26, 2021

zachmargolis Aug 26, 2021

Choose a reason for hiding this comment

aduth Aug 26, 2021

Choose a reason for hiding this comment

zachmargolis Aug 26, 2021

Choose a reason for hiding this comment

zachmargolis Aug 26, 2021

Choose a reason for hiding this comment

aduth Aug 26, 2021

Choose a reason for hiding this comment

zachmargolis Aug 26, 2021

Choose a reason for hiding this comment

aduth commented Aug 25, 2021 •

edited

Loading

aduth commented Aug 26, 2021 •

edited

Loading