Convert pathogens to app router [#1066] #1124

genehack · 2025-02-26T19:21:45Z

Description of proposed changes

Converts /pathogens and components used on that page to App Router style.

Converted components:

ErrorBoundary
ExpandableTiles
ListResources (and included sub-components, ResourceGroup, IndividualResource, etc)
Spinner

Note that all of these were done in two stages: first a move or copy of the old Pages Router style code to the new App Router location — which sometimes, inevitably, leaves the code in a broken state — and then a subsequent commit to convert to the App Router style/code layout.

Please see individual commits for extra details around some porting decisions.

Related issue(s)

Closes #1066

Checklist

Checks pass
Check if changes affect the resource index JSON revision

Note: this will leave the site in a broken state; doing this to ensure better Git tracking of file renames.

Note: this is a copy, not a move, as `<ExpandableTiles>` is also used in the `splash` and `Groups` components in addition to the `ListResources` component that is consumed by the `/pathogens` page.

Note: this is a copy, not a move, as `<ErrorBoundary>` is used elsewhere in the site, in addition to the `ListResources` component that is consumed by the `/pathogens` page.

Note: this is a copy, not a move, as `<ListResources>` is used elsewhere in the site, in addition to the `/pathogens` page.

Note: despite the placement of the component outside the <ListResources> component, it is only used within that component. I considered relocating it to `static-site/components/list-resources/spinner.tsx` but elected to maintain it outside of the <ListResources> component to better support future re-use, which seems not unlikely.

static-site/components/list-resources/individual-resource.tsx

jameshadfield

I tested out (in dev mode) and the page & styles look fine, although I couldn't test the modal due to the context error Jover noted. I didn't notice any poor on-hover box positioning, but you may have already fixed that bug?

I didn't really review the code as (from skimming) the majoriy of changes seem to be whitespace / splitting across new lines / changing comment styles etc. If there are sections of code which you'd like me to take a look at let me know and I'd be happy to; perhaps css modules?

jameshadfield · 2025-02-26T20:54:46Z

static-site/components/list-resources/use-data-fetch.ts

-import { useState, useEffect } from 'react';
-import { Group, Resource, ResourceListingInfo } from './types';
-import { InternalError } from '../ErrorBoundary';
-
-
-/**
- * Uses the provided callback to fetch resources and parse those
- * resources into the `pathVersions` and `pathPrefix` values. The
- * callback is expected (and encouraged!) to throw() on any errors.
- *
- * Continues on to parse the `pathVersions`/`pathPrefix` data
- * structures into an array of groups, each representing a "pathogen"
- * and detailing the available resources for each, and the available
- * versions (snapshots) for each of those resources. In the case of
- * un-versioned resources, versions will be a zero-length array (i.e.,
- * `[]`)
- *
- * The current implementation defines `versioned: boolean` for the
- * entire API response, however in the future we may shift this to the
- * API response and it may vary across the resources returned.
- */
-export function useDataFetch(
+"use client";
+
+import { useState, useEffect } from "react";
+
+import { InternalError } from "../error-boundary";
+
+import { Group, Resource, ResourceListingInfo } from "./types";
+
+// Uses the provided callback to fetch resources and parse those
+// resources into the `pathVersions` and `pathPrefix` values. The
+// callback is expected (and encouraged!) to throw() on any errors.
+//
+// Continues on to parse the `pathVersions`/`pathPrefix` data
+// structures into an array of groups, each representing a "pathogen"
+// and detailing the available resources for each, and the available
+// versions (snapshots) for each of those resources. In the case of
+// un-versioned resources, versions will be a zero-length array (i.e.,
+// `[]`)
+//
+// The current implementation defines `versioned: boolean` for the
+// entire API response, however in the future we may shift this to the
+// API response and it may vary across the resources returned.
+export default function useDataFetch(


Is this reformatting done by hand or an automated tool? It seems... unnecessary [1]. Looking at this highlighted block as an example, there are only 2 changes - addition of "use client" and export default vs export - but we've changed all 22 lines.

There are examples throughout this PR. The downside is that tracking changes through git blame (and the GitHub interface into that) becomes much much harder [2]. I'm not sure what the upside is - consistency?

[1] also unnecessary to spend time switching it back I guess

[2] Although a preceeding commit (365f622) added this file without any history (i.e. a duplication of an existing file, not a rename), so git blame isn't going to be useful in any case

Yes, the intention is consistency. Some of the reformatting is by hand; some of it is semi-automated via prettier and similar tooling.

I'm treating all of the new App Router files (so, things under /static-site/app and /static-site/components) as effectively completely new files, and trying to have them all be consistent with each other in terms of layout and style, as well as consistent with "standard" Next.js layout and approach.

The last big chunk of work I did, I did not include the "starting" version of the file, and there were complaints about breaking the history. This time, when I had things where I had to make a copy (because the existing version of the file needs to keep working to support as-yet un-ported Page Router-based pages), I included an initial commit that contains the "starting" version of the file, without any changes, as that felt like the best way to indicate what had happened.

It is very unclear to me what approach would satisfy everybody — I somewhat suspect there isn't one. If people have concrete suggestions on better ways to do things, I'm happy to hear them; feedback that amounts to "I don't like this" without a suggestion of what would be better is not helpful to me.

Overall, enough of the specifics and the finer details of implementation have changed and been cleaned up as part of this port, that it seems highly unlikely to me that anybody would ever end up walking the history back into the Pages Router version.

I don't care to weigh in here on how to do this conversion more broadly, but I do want to point out that here (and generally) the change of comment style from

-/** - * Uses the provided callback to fetch resources and parse those +// Uses the provided callback to fetch resources and parse those

turns what was a JSDoc comment into a non-JSDoc comment.

Now, we're not currently doing anything with those JSDoc comments beyond reading them with our own eyes (although maybe some your IDEs are automatically?), but the convention in this code base has been to write JSDoc comments for modules, functions, classes, etc.

I don't care to weigh in here on how to do this conversion more broadly, but I do want to point out that here (and generally) the change of comment style from

-/** - * Uses the provided callback to fetch resources and parse those +// Uses the provided callback to fetch resources and parse those

turns what was a JSDoc comment into a non-JSDoc comment.

They didn't read as JSDoc comments to me, because they don't have any actual JSDoc-style annotations, like @constructor or @param; they just looked like C++-style block comments in front of some (not all) of the functions. (I do see JSDoc comments with annotations in other parts of the repo, but I've kinda being viewing the static-site/ directory and its contents as a distinct thing from the rest of the codebase.

Now, we're not currently doing anything with those JSDoc comments beyond reading them with our own eyes (although maybe some your IDEs are automatically?), but the convention in this code base has been to write JSDoc comments for modules, functions, classes, etc.

I guess my question there is, is there still a use for JSDoc-style commentary in a Typescript codebase? I think of JSDoc as an attempt to layer type hints onto plain old JS.

although maybe some your IDEs are automatically [doing something with these JSDoc comments]

vs-code picks up these JS-Doc style comments and I find it helpful. See before/after screenshots below. I'd like the JS-Doc style comments to remain.

@victorlin do you think we should be adding @param x ... to function docs if the param is itself typed with a description in the underlying type?

Not Victor, but noting: none of the @param annotations I added are to document the parameter types; they all add explanatory commentary about what the parameter is for, or means. In some cases, this involves describing the type (sometimes, you have to call a boolean a boolean), but that's not the main thrust.

none of the @param annotations I added are to document the parameter types

Yeah, I get that, I'm talking about the duplication of the description between the one found within the type definition and the one found in the JSDoc of the calling function. This is not blocking, just a question. We should, however, change the comment style back to JSDoc within the type defs.

none of the @param annotations I added are to document the parameter types

Yeah, I get that, I'm talking about the duplication of the description between the one found within the type definition and the one found in the JSDoc of the calling function.

Ah, yeah. I will note that -- broadly and generally speaking -- the various bits of Typescript-specific stuff that have been added (custom types, interfaces, etc.) are not documented at all, beyond the inherent documentation of how things are named.

This is not blocking, just a question. We should, however, change the comment style back to JSDoc within the type defs.

yep, that's been done.

… the function ListResources(props: ListResourcesProps) doesn't give us any of these niceties because it doesn't destructure props in the signature.

I'm struggling to understand. Isn't everything tucked away in props because the individual properties are not relevant in the component directly? Even if there was a reference to something like props.defaultGroupLinks, the info-on-hover would be available:

@victorlin do you think we should be adding @param x ... to function docs if the param is itself typed with a description in the underlying type?

We can avoid @param lines entirely for TypeScript functions in favor of JSDoc descriptions on the original property definitions, even if it's inline within the function signature (example in Auspice). Right now there is some duplication and inconsistency:

nextstrain.org/static-site/components/list-resources/index.tsx

Lines 131 to 136 in 279f3b4

* @param versioned - boolean for whether the resources are versioned

* @param elWidth - width of the element

* @param quickLinks - list of `QuickLink` objects for the resources

* @param defaultGroupLinks - boolean for if the group name is a url

* @param groupDisplayNames - apping from group name -> display name

* @param tileData - the tiles for the resources

nextstrain.org/static-site/components/list-resources/index.tsx

Lines 45 to 62 in 279f3b4

interface ListResourcesProps {

versioned: boolean;

quickLinks: QuickLink[];

/**

* Should the group name itself be a url? (which we let the server

* redirect)

*/

defaultGroupLinks: boolean;

/** Mapping from group name -> display name */

groupDisplayNames: Record<string, string>;

tileData: FilterTile[];

/** this is currently unused */

resourceType: string;

}

I would address with something like e3563b0

static-site/components/list-resources/individual-resource.tsx

joverlee521

Left small nits and comments, but overall the ported /pathogens page works for me.

static-site/components/list-resources/modal.tsx

static-site/components/list-resources/group-and-resource-links.module.css

* add `use client` because of use of `useState` * rename `Tile` type to `GenericTileBase`; rename `AnyTile` to `Tile` * converted styled components to CSS module * swap various prop-driven styled components bits to dynamic inline styles

* move `div.errorContainer` into global CSS file because it is used in multiple places

* segregate types and functions * update formatting

* adjust leading/trailing whitespace

joverlee521

Skimmed over force-push changes and did another review in the test-app.

Overall changes look good, I only flagged minor text formatting and styling issues.

static-site/components/list-resources/resource-group.module.css

static-site/components/list-resources/modal.tsx

jameshadfield

Tested locally (dev mode) and things work well, but didn't inspect the styles as in depth as Jover did.

* move styling from styled components to CSS module * refactor <ResourceModal> to `useContext` and `useCallback` to build `dismissModal()` function, rather than passing it in — this was necessary because all props of React Client Components need to be serializable (i.e., not functions), and <ResourceModal> needs to "use client" because it depends on `useState` and `useEffect` * refactor the `_draw` function out into a distinct file (`modal_draw.js`) to reflect the lack of typing in that code (I briefly explored converting that file to Typescript and concluded the juice was not worth the squeeze) * note that this also means we don't need to add @types/d3 to devDependencies

* export component as a function * convert to Typescript; light reformatting

* Refactor <Name>, <ResourceLinkWrapper>, and <ResourceLink> into <IndividualResourceLink>, and migrate into distinct file; in the future, <GroupLink> will also be refactored into that file, which is reflected in the `group-and-resource-links` file name * Refactor <IconContainer> into distinct file, rework to avoid need to pass non-serializable prop * Refactor <IndividualResource> and associated components from Styled to CSS Module * Refactor <IndividualResource> component to be passed `gapWidth` prop, which supports cleaner separation of concerns with where that constant is defined and used * Refactor <TooltipWrapper> into a distinct file, because it doesn't need to be a React Client component

* Refactored styles from structured component to CSS Module * Refactored <GroupLink> and <IndividualQuickLink> components out into `group-and-resource-links.tsx` * Refactored <ResourceGroup> and contained components

* convert `export const`` to `export default function` * add "use client" * refactor and update code

] * formatting consistency with other code

* convert to React Client Component * refactor code lightly

* convert from styled comp to css module * convert const fat-arrows to functions * update imports, etc.

This moves the resource listing callback into a distinct file; this is because Next.js Pages are limited in the types of methods that can be exported from them. The resource listing code was moved into a distinct method, which is imported by the `<ListResources>` component, so that this page file can remain a React Server Component, which allows for the Metadata API to be used to generate content in the page `<head>` during SSR, prior to hydration.

genehack added 4 commits February 25, 2025 14:42

Initial move of /pathogens page into App Router style [#1066]

832e834

Note: this will leave the site in a broken state; doing this to ensure better Git tracking of file renames.

Initial copy of ExpandableTiles component into App Router style [#1066]

52639e4

Note: this is a copy, not a move, as `<ExpandableTiles>` is also used in the `splash` and `Groups` components in addition to the `ListResources` component that is consumed by the `/pathogens` page.

Initial copy of ErrorBoundary component into App Router style [#1066]

4ad805e

Note: this is a copy, not a move, as `<ErrorBoundary>` is used elsewhere in the site, in addition to the `ListResources` component that is consumed by the `/pathogens` page.

Initial copy of ListResources component into App Router style [#1066]

365f622

Note: this is a copy, not a move, as `<ListResources>` is used elsewhere in the site, in addition to the `/pathogens` page.

nextstrain-bot temporarily deployed to nextstrain-s-convert-pa-9ilhrn February 26, 2025 19:22 Inactive

genehack force-pushed the convert-pathogens-to-app-router-1066 branch from bfd05fb to 2c2d562 Compare February 26, 2025 21:46

nextstrain-bot temporarily deployed to nextstrain-s-convert-pa-heatct February 26, 2025 21:46 Inactive

genehack force-pushed the convert-pathogens-to-app-router-1066 branch from 2c2d562 to 042d526 Compare February 26, 2025 23:10

nextstrain-bot temporarily deployed to nextstrain-s-convert-pa-2bxl9d February 26, 2025 23:10 Inactive

genehack mentioned this pull request Feb 26, 2025

Convert static-site from Pages Router to App Router #1052

Open

32 tasks

joverlee521 reviewed Feb 27, 2025

View reviewed changes

static-site/components/list-resources/individual-resource.tsx Show resolved Hide resolved

jameshadfield requested changes Feb 27, 2025

View reviewed changes

genehack force-pushed the convert-pathogens-to-app-router-1066 branch from 042d526 to da860b4 Compare February 27, 2025 23:43

genehack temporarily deployed to nextstrain-s-convert-pa-2bxl9d February 27, 2025 23:43 Inactive

genehack mentioned this pull request Feb 28, 2025

Convert sars forecasts to app router [#1125] #1126

Merged

2 tasks

joverlee521 approved these changes Feb 28, 2025

View reviewed changes

static-site/components/list-resources/modal.tsx Outdated Show resolved Hide resolved

static-site/components/list-resources/group-and-resource-links.module.css Show resolved Hide resolved

static-site/components/list-resources/group-and-resource-links.module.css Outdated Show resolved Hide resolved

genehack force-pushed the convert-pathogens-to-app-router-1066 branch from da860b4 to e2a9855 Compare March 3, 2025 23:39

genehack temporarily deployed to nextstrain-s-convert-pa-2bxl9d March 3, 2025 23:40 Inactive

genehack added 4 commits March 6, 2025 12:27

Port <ErrorBoundary> component to App Router style [#1066]

aa5ffd1

* move `div.errorContainer` into global CSS file because it is used in multiple places

Port <ListResources> types file to App Router style [#1066]

ee705d7

* segregate types and functions * update formatting

Port <ListResources> dodge.js file to App Router [#1066]

020ea7a

* adjust leading/trailing whitespace

genehack force-pushed the convert-pathogens-to-app-router-1066 branch from e2a9855 to d6f0962 Compare March 6, 2025 20:27

genehack had a problem deploying to nextstrain-s-convert-pa-2bxl9d March 6, 2025 20:28 Failure

genehack force-pushed the convert-pathogens-to-app-router-1066 branch from d6f0962 to 4cfe238 Compare March 6, 2025 20:41

genehack had a problem deploying to nextstrain-s-convert-pa-2bxl9d March 6, 2025 20:41 Failure

genehack force-pushed the convert-pathogens-to-app-router-1066 branch from 4cfe238 to feb55a3 Compare March 6, 2025 20:56

genehack temporarily deployed to nextstrain-s-convert-pa-2bxl9d March 6, 2025 20:56 Inactive

genehack requested a review from jameshadfield March 7, 2025 00:45

joverlee521 requested changes Mar 7, 2025

View reviewed changes

jameshadfield approved these changes Mar 9, 2025

View reviewed changes

genehack added 9 commits March 10, 2025 14:27

Port <Spinner> component to App Router style [#1066]

9a289d1

* export component as a function * convert to Typescript; light reformatting

Port <ResourceGroup> component to App Router style [#1066]

4ce3f3c

* Refactored styles from structured component to CSS Module * Refactored <GroupLink> and <IndividualQuickLink> components out into `group-and-resource-links.tsx` * Refactored <ResourceGroup> and contained components

Port useSortAndFilter() to App Router style [#1066]

b686a1c

* convert `export const`` to `export default function` * add "use client" * refactor and update code

Port createFilterOptions and useFilterOptions to App Router style [#1066

1c125bd

] * formatting consistency with other code

Port useDataFetch to App Router style [#1066]

5426b24

* convert to React Client Component * refactor code lightly

Port <ListResources> to App Router style [#1066]

4e65913

* convert from styled comp to css module * convert const fat-arrows to functions * update imports, etc.

genehack force-pushed the convert-pathogens-to-app-router-1066 branch from feb55a3 to 3a75a9a Compare March 10, 2025 21:28

genehack temporarily deployed to nextstrain-s-convert-pa-2bxl9d March 10, 2025 21:28 Inactive

genehack merged commit be1c8c1 into master Mar 10, 2025
8 checks passed

genehack deleted the convert-pathogens-to-app-router-1066 branch March 10, 2025 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert pathogens to app router [#1066] #1124

Convert pathogens to app router [#1066] #1124

genehack commented Feb 26, 2025 •

edited

Loading

jameshadfield left a comment

jameshadfield Feb 26, 2025

genehack Feb 28, 2025

tsibley Feb 28, 2025

genehack Feb 28, 2025

jameshadfield Mar 2, 2025

genehack Mar 10, 2025

jameshadfield Mar 10, 2025

genehack Mar 10, 2025

victorlin Mar 10, 2025

victorlin Mar 10, 2025 •

edited

Loading

joverlee521 left a comment

joverlee521 left a comment

jameshadfield left a comment

	* @param versioned - boolean for whether the resources are versioned
	* @param elWidth - width of the element
	* @param quickLinks - list of `QuickLink` objects for the resources
	* @param defaultGroupLinks - boolean for if the group name is a url
	* @param groupDisplayNames - apping from group name -> display name
	* @param tileData - the tiles for the resources

	interface ListResourcesProps {
	versioned: boolean;
	quickLinks: QuickLink[];

	/**
	* Should the group name itself be a url? (which we let the server
	* redirect)
	*/
	defaultGroupLinks: boolean;

	/** Mapping from group name -> display name */
	groupDisplayNames: Record<string, string>;

	tileData: FilterTile[];

	/** this is currently unused */
	resourceType: string;
	}

Convert pathogens to app router [#1066] #1124

Convert pathogens to app router [#1066] #1124

Conversation

genehack commented Feb 26, 2025 • edited Loading

Description of proposed changes

Related issue(s)

Checklist

jameshadfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorlin Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

joverlee521 left a comment

Choose a reason for hiding this comment

joverlee521 left a comment

Choose a reason for hiding this comment

jameshadfield left a comment

Choose a reason for hiding this comment

genehack commented Feb 26, 2025 •

edited

Loading

victorlin Mar 10, 2025 •

edited

Loading