feat: nextrouter pkg to handle nextjs routing rules #167

bryphe-coder · 2022-02-05T18:06:42Z

An issue came up last week... our embed.go strategy doesn't handle dynamic NextJS-style routes! This is a blocker, because I'm aiming to set up CD on Monday, and the v2 UI makes heavy use of dynamic routing.

As a potential solution, this implements a go pkg nextrouter that serves html files, but respecting the dynamic routing behavior of NextJS:

Files that have square brackets - ie [providers] provide a single-level dynamic route
Files that have [[... prefix - ie [[...any]] - are catch-all routes.
Files should be preferred over folders (ie, providers.html is preferred over /providers)
Fixes the trailing-slash bug we hit in the previous embed strategy

This also integrates with slog.Logger for tracing, and handles injecting template parameters - a feature we need in v1 and v2 to be able to inject stuff like CSRF tokens.

This implements testing by using an in-memory file-system, so that we can exercise all of these cases.

In addition, this adjust V2's embed.go strategy to use nextrouter, which simplifies that file considerably. I'm tempted to factor out the secureheaders logic into a separate package, too.

If this works OK, it could be used for V1 too (although that scenario is more complex due to our hybrid-routing strategy). Based on our FE variety meeting, there's always a chance we could move away from NextJS in v1 - if that's the case, this router will still work and be more tested than our previous strategy (it just won't make use of dynamic routing). So I figured this was worth doing to make sure we can make forward progress in V2.

codecov · 2022-02-05T18:09:33Z

Codecov Report

Merging #167 (e4d031f) into main (61b3909) will increase coverage by 3.37%.
The diff coverage is 74.35%.

@@            Coverage Diff             @@
##             main     #167      +/-   ##
==========================================
+ Coverage   64.66%   68.03%   +3.37%     
==========================================
  Files          58      111      +53     
  Lines         648     6004    +5356     
  Branches       68       68              
==========================================
+ Hits          419     4085    +3666     
- Misses        217     1526    +1309     
- Partials       12      393     +381

Flag	Coverage Δ
unittest-go-macos-latest	`65.87% <74.35%> (?)`
unittest-go-ubuntu-latest	`67.49% <74.35%> (?)`
unittest-go-windows-latest	`65.82% <74.35%> (?)`
unittest-js	`64.66% <ø> (ø)`

Impacted Files	Coverage Δ
site/embed.go	`83.14% <31.25%> (ø)`
site/nextrouter/nextrouter.go	`79.13% <79.13%> (ø)`
coderd/coderd.go	`94.50% <100.00%> (ø)`
codersdk/workspaces.go	`73.73% <0.00%> (ø)`
httpmw/userparam.go	`76.66% <0.00%> (ø)`
database/migrate.go	`52.38% <0.00%> (ø)`
provisionerd/provisionerd.go	`68.29% <0.00%> (ø)`
database/pubsub_memory.go	`87.17% <0.00%> (ø)`
httpmw/provisionerjobparam.go	`78.37% <0.00%> (ø)`
peer/netconn.go	`30.00% <0.00%> (ø)`
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61b3909...e4d031f. Read the comment docs.

kylecarbs

This is unfortunate but seems necessary.

Instead of this, could we do the following:

Enable trailingSlash in next.config.js so all files are named index.html.
Manually specify parameterized routes. This sucks, but I'm nervous to add a few hundred lines of routing code (albeit tested) when we could add like: router.Match("/projects/{organization}/{project}", serveStaticHandler)

Thoughts?

bryphe-coder · 2022-02-05T18:33:00Z

Manually specify parameterized routes. This sucks, but I'm nervous to add a few hundred lines of routing code (albeit tested) when we could add like: router.Match("/projects/{organization}/{project}", serveStaticHandler)

I thought about this after our discussion and FE variety yesterday - but I'm not a fan, because it seems too hard to maintain. The problem is our development environment with ./develop works w/o having to specify paths, so it's hard to know / remember to add a route.

Another thing is - our previous routing code wasn't tested at all, so a lot of the previous routing code is removed and covered with tests. I added 225 lines of routing code - covered with tests - but removed 185 lines of code that wasn't tested at all. So it's really only net +40 lines of tested code.

kylecarbs

Wonderful tests! I went hard into nits for this, but overall this is wonderfully tested and structured code 🥳🥳🥳

kylecarbs · 2022-02-05T19:17:09Z

nextrouter/nextrouter.go

+	"cdr.dev/slog"
+)
+
+// Options for configuring a nextrouter


I don't want this to be confused with our primary app routing logic.

For that reason, what do you think about this being in site like it was before? It could even be at the root if you feel that's clean enough.

Hmm, I'd prefer to keep it in its own package so it's clear that it is stand-alone - how about I move it to site/nextrouter?

Moved in 19fcfac

Makes sense to me!

nextrouter/nextrouter.go

kylecarbs · 2022-02-05T19:52:43Z

BTW I agree with your comment. It is a bit too sketchy. After reading through the code extensively my POV changed!

kylecarbs

An absolutely wonderful change!

bryphe-coder · 2022-02-09T01:01:07Z

Thanks for reviewing, @kylecarbs !

greyscaled · 2022-02-09T01:14:14Z

site/embed.go

+	nextRouterHandler, err := nextrouter.Handler(filesystem, &nextrouter.Options{
+		Logger:           logger,
+		TemplateDataFunc: templateFunc,


praise: I really like that the nextjs stuff is isolated into a self-contained module.

greyscaled · 2022-02-09T01:16:52Z

site/nextrouter/nextrouter.go

@@ -0,0 +1,238 @@
+package nextrouter


nit: Can you add a package-level comment about what this module is? It would need to go in nextrouter/doc.go.

See here: https://github.com/coder/m/blob/master/product/coder/pkg/vm/ec2/doc.go

I'm happy to contribute this as my first coder/coder PR if you're ok with that 😂

I like doc.go, but only if we can navigate this repo on https://go.dev. Otherwise it's generally gone unmaintained D:

Sure, that'd be great to contribute @vapurrmaid !

bryphe-coder added 29 commits February 5, 2022 00:35

Add in-memory fs example

dd6d315

Integrate test server too

33897e5

Some initial test cases for files at root

b8d092c

Serve contents from filesystem

9949261

Factor router out

fadc0b6

Add recursive router building

771f39d

Hook up recursive case

78406b1

Refactor to handle different file-serving cases

50c72ba

Handle case w/o html extension

7a98a48

Handle redirecting to index.html at root

1eb2402

Add test case for nested path

b0fd9a8

Remove now-unnecessary smoke test

a506148

Some clean up

b59e2a8

Refactor to use http.ServeContent

ff75695

Simplify router construction

a410396

rtr -> router

15c38b9

Handle serving non-html files

c813b5e

Handle trailing-slash case

317c040

Implement dynamic routing

133e452

Add test case to verify static paths are preferred over dynamic paths

6d8f412

Handle dynamic routing for folders

a3ecd7f

Handle catch-all routes

700c9ef

Start plumbing in a way to inject template parameters

05a369e

Start adding plumbing for templates

03a20c1

Add template functionality

199d468

Set up 404 handling

ca515e4

Use nextrouter package in site, which simplifies it quite a bit

6072bad

Update interface to take logger

826eaf5

Additional comments

dac34f5

bryphe-coder self-assigned this Feb 5, 2022

Fix embed_test

92d4eb0

bryphe-coder requested a review from kylecarbs February 5, 2022 18:11

Remove now-unused noop template func

5262b7f

kylecarbs reviewed Feb 5, 2022

View reviewed changes

kylecarbs approved these changes Feb 5, 2022

View reviewed changes

bryphe-coder added 11 commits February 8, 2022 04:41

Merge main

33342e5

Switch route addition from info -> debug log level

750bec9

Invert logic to remove layer of indentation

c15b046

Rename buildRoutes -> registerRoutes

c17fc15

TemplateDataFunc -> HTMLTemplateHandler

34e4f46

Add return to reduce indentation in serveFile

0743503

Remove call to remove file extension

4585c13

Switch to register 404, return error and warn at toplevel

8f2b47d

Move ./nextrouter -> ./site/nextrouter

19fcfac

Return error from handler

8343923

Change dynamic route logging from Info -> Debug

4a5c353

kylecarbs approved these changes Feb 8, 2022

View reviewed changes

Merge main

e4d031f

bryphe-coder merged commit 78bf4c6 into main Feb 9, 2022

bryphe-coder deleted the bryphe/feat/nextrouter-pkg branch February 9, 2022 01:01

greyscaled reviewed Feb 9, 2022

View reviewed changes

ketang unassigned bryphe-coder Mar 22, 2022

feat: nextrouter pkg to handle nextjs routing rules #167

feat: nextrouter pkg to handle nextjs routing rules #167

Conversation

bryphe-coder commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kylecarbs left a comment

Choose a reason for hiding this comment

Uh oh!

bryphe-coder commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kylecarbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kylecarbs commented Feb 5, 2022

Uh oh!

kylecarbs left a comment

Choose a reason for hiding this comment

Uh oh!

bryphe-coder commented Feb 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bryphe-coder commented Feb 5, 2022 •

edited

Loading

codecov bot commented Feb 5, 2022 •

edited

Loading

bryphe-coder commented Feb 5, 2022 •

edited

Loading