Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade VZE to NextJS #1603

Open
wants to merge 440 commits into
base: main
Choose a base branch
from
Open

Upgrade VZE to NextJS #1603

wants to merge 440 commits into from

Conversation

johnclary
Copy link
Member

@johnclary johnclary commented Nov 15, 2024

Associated issues

Here is the working prototype of the VZE in NextJS! I am envisioning that folks spend time with the code and add their questions/comments via review, and from there we convene a meeting to discuss next steps and scope issues.

Testing

  1. Star your local stack and apply migrations and metadata
  2. Start the nextjs app: see the README for instructions.
  3. If you've tested this app previously, you may need to remove the old version of crashesListViewQueryConfig from your localStorage
  4. What to test: you should be able to use the app without breaking it! Moreover, the current list of working features is in the readme, here.
  5. Code review: all feedback is welcome, and especially related to the libraries i've chosen and overall project structure.
  6. I have quite a few doubts and questions about the designing of the types, and a very important todo (i think) is to add in Zod schema validation to generate our core types and wrap all our API requests.

Ship list

  • Check migrations for any conflicts with latest migrations in main branch
  • Confirm Hasura role permissions for necessary access
  • Code reviewed
  • Product manager approved

children: React.ReactNode;
}) {
return (
// <Html lang="en" style={{fontSize: "14px"}} data-bs-theme="dark">
Copy link
Member Author

@johnclary johnclary Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i left this here bc the dark theme is glorious to dev on. would be fun to spin off an issue to make a UI toggle for it 🌚

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after switching back to light mode a while back, maybe it is time for me to find my way back to the dark side. 😈

}) {
return (
// <Html lang="en" style={{fontSize: "14px"}} data-bs-theme="dark">
<html lang="en" style={{ fontSize: "14px" }}>
Copy link
Member Author

@johnclary johnclary Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my chrome browser's font size is 16, and the ACC designs go down to 12, maybe even smaller. it's an accessibility issue that needs more consideration 👀

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw this is what MUI does - https://mui.com/material-ui/customization/typography/#font-size which also seems to be the default of Bootstrap too - 1rem. Idk why the font looks so large with this override removed. But, i agree that it is something that needs more thought and can be adjusted globally later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks pointing to MUI's approach—interesting that they default to 14px.

My understanding of the way it works is that 1rem = whatever the browser default is, unless specified in the html tag, in which case 1rem becomes the specified value.

* Each object in the array takes the format
* { field: <property name>, old: <old value>, new: <new value> }
*/
const getDiffArray = <T extends Record<string, unknown>>(
Copy link
Member Author

@johnclary johnclary Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting this typed correctl was a lot of trial and error, even though the result looks simple. these kinds of data-munging tasks are what i've found to be the most onerous to convert to typescript.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof yeah, glad you figured this out! i need to learn more about this Record type.

It did make me think about whether we would want to add a linter rule to enforce no any type. https://typescript-eslint.io/rules/no-explicit-any/ I think that code reviews would keep us on track, but it could be worth thinking about now.

Copy link
Member Author

@johnclary johnclary Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i'm 100% on board for no any types allowed—and it appears to be enabled by default, at least in local dev.

const myVar: any = 2;
# npm run build
10:13  Error: Unexpected any. Specify a different type.  @typescript-eslint/no-explicit-any

* Prettify the user name if it is `cris`
*/
const formatUserName = (userName: string): string =>
userName === "cris" ? "TxDOT CRIS" : userName;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i just want to point out that this function has a return type defined (string). typescript will infer the return type based on what your code returns, but as a best practice i think we want to strive for explicitly naming the return type.

there are some spots where i failed to do this and i'm keeping an eye out for them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like it! this is something that i could learn about for sure.

Copy link
Member Author

@johnclary johnclary Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auth is implemented fully client-side, like the existing VZE.

basically, the flow is that a user logs into Auth0, the JWT is stored in localstorage, and the app uses that JWT to make calls to the Hasura API. Hasura validates the JWT and permissions. So our app is just a passthrough for the authentication. It manages the task of getting a token and refreshing it.

Auth0 does have a server-side lib, nextjs-auth0, which is specific to the app router. The big difference with that approach is that our own nextjs server (Netlify or Vercel) actually validates the JWT that is exchanged. To make that possible, we need to store our JWT secret in the app environment. (whereas the client-side setup gets by on the publicly-shareable env vars).

If we want to authenticate a server-side function, we'll need to adopt the nextjs-auth0 package. For example, if we wanted to replace the CR3 API. But for our current needs there is no benefit to using the nextjs Auth0 package, and in fact it adds the overhead of needing to manage a very sensitive secret in our local setup.

That's my digest of the tradeoffs as I understand them.

@@ -0,0 +1,29 @@
"use client";
import { Auth0Provider } from "@auth0/auth0-react";
Copy link
Member Author

@johnclary johnclary Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

React context is a client-only feature, which is where the change in mental model really hit home for me.

If you're using a 3rd-party library that relies on context (like Auth0), you need to wrap it in "use client", unless the author does it for you. Here are the nextjs docs that discuss that.

permanent: true,
},
];
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file was generated by create next app and i added this redirect.

},
"dependencies": {
"@auth0/auth0-react": "^2.2.4",
"@types/lodash": "^4.17.13",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only lodash function we're using at the moment is isEqual, and as far as i can tell the bundler is doing a great job tree-shaking it.

i don't know what state the lodash project is in. i think v5 has been in the works for a very long time...

"next": "14.2.13",
"react": "^18",
"react-bootstrap": "^2.10.4",
"react-datepicker": "^7.5.0",
Copy link
Member Author

@johnclary johnclary Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this datepicker gets the job done, but it looks really dated (ha ha). i have not found a framework-agnostic alternative but didn't look very hard.

Copy link
Member Author

@johnclary johnclary Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes, here we go: https://github.com/gpbl/react-day-picker. this one is used by shadcn/ui.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as long as we don't go with the one that I picked for VZV! 🙈

@@ -0,0 +1,24 @@
import { format, parseISO } from "date-fns";

// const formatCostToDollars = (cost: number | null) =>
Copy link
Member Author

@johnclary johnclary Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i left this here because we'll need it to render comp costs on the location details pages

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the change log was copied verbatim from VZE, with the exception that it now renders even if there are no change records at all (fixing a weird quirk of VZE local dev where the change log is hidden)

app/env_template Outdated
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our env files are no longer committed to the repo 🎉

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be able to accomplish most of our theming here. bootstrap docs are pretty good about how to accomplish this, but getting all of the sass variable overrides correct will take some work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i did not create this file 🤖

/**
* Generic component which renders editable fields in a Card
*/
export default function DataCard<T extends Record<string, unknown>>({
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component replaces the DataTable component in VZE. The only significant difference is that it fetches lookup table values when a lookup field is toggled into its edit state, as opposed to fetching all lookup values when the crash details page loads.

The lookup queries are cached, and in general are quick to load, but we need to test this with our power users, who may notice a slight performance degradation.

* The base definition of a database column referenced by our app — where
* <T> should be one of our core record types (Crash, Person, etc)
*/
export type ColBaseDef<T extends Record<string, unknown>> = {
Copy link
Member Author

@johnclary johnclary Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ColBaseDef is the smallest building block of our types coming out of the database, and we can use them to build up any time of record: crashes, units, etc.

Also, you can see that I have extended from Record<string, unknown>> type, and i do that in many places. I think that's fine and good, but because it is used so often across the codebase we should probably give it a name and export it. something like <RecordWithStringKeys>. that's all it is: an object with keys that are always strings.

this will make more sense after reading about the Record utility type: https://www.typescriptlang.org/docs/handbook/utility-types.html#recordkeys-type :)

Copy link
Member Author

@johnclary johnclary Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i started collecting most types here, but have also left component-specific types and interfaces near to their components. i don't feel strongly about how to organize these, but i do think we'll want to split out entity-specific types like crash, unit, etc. into their own files.

I also think that bringing zod into the mix might significantly change how we write out these types, so i didn't want to overthink it. See my note in zodExample.ts.

@@ -0,0 +1,194 @@
import { useState, useCallback, useEffect, useMemo } from "react";
import useSWR, { SWRConfiguration } from "swr";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all of our data fetching is managed with SWR and graphql-request.

SWR is a Vercel-managed library designed specifically for client-side data fetching, and I really like working with it.

The Apollo setup for nextjs app router looks fairly involved, and i haven't messed with it. I'm open to it, but I stuck with SWR since it's familiar to me and is part of the nextjs ecosystem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think swr is great, and I don't see anything it doesn't cover that we got out of Apollo. imo Apollo client probably gives more than we need, and we also ran into some weird problems with fetching data for csv exports. Actually, lazy querying is something that we'll probably need, but maybe it is covered?

Copy link
Member Author

@johnclary johnclary Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, as far as i have been able to test it, we can do the lazy querying with the conditional data fetching functionality and alternatively the useSWRInfinite hook — which i was recently looking at for paginating through the users API.

* Dont refetch on network recon
*/
revalidateOnReconnect: false,
};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are just my opinions to cut back on unnecessary API calls—SWR really loves to make API calls. i encourage y'all to check out the various settings available 🧪

* dicussion here: https://community.auth0.com/t/getting-the-jwt-id-token-from-auth0-spa-js/28281/3
*/
const idToken = await getIdTokenClaims();
const token = idToken?.__raw || "";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this hacky-feeling code is in the old VZE as well. more research needed here.

query ? [query, variables] : null,
fetchWithAuth,
{ ...DEFAULT_SWR_OPTIONS, ...(options || {}) }
);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here's where we call the actual useSWR hook—and a few important things to note:

  • the first argument is the value that SWR uses as the cache key. so each combination of query + variables gets cached.
  • passing null to the first argument is like an off switch, and enables conditional data fetching. this comes in super handy when you don't want to start a request until you have, e.g, a user's email address from the auth hook.

Copy link
Contributor

@mddilley mddilley Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may have answered my lazy query question. 👀

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah!

Copy link
Member Author

@johnclary johnclary Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the queryBuilder replaces the graphqlAbstract in the VZE. basically, it takes a giant config object and spits out a graphql query. it puts the work of modifying the query configuration in the hands of UI components.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i said it in the last meeting, but i like this strategy to build what we need now and extend later if needed and NOT try to cover all of the functionality coming out of the Hasura API. 🙌 It is a nice iteration that builds of our knowledge gained through the first go-around and Moped refactor. Very cool stuff.

| "_is_null"
| "_ilike"
| "_in"
| "_nin";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasura supports more operators, but i've only added those that we're actually using. we can add more as needed 👍

* At runtime, we would use the zod schema to validate api calls,
* and during development we have typechecking in sync with
* what zod is validating.
*/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't know if the below code makes much sense, but these are my thoughts on how we might approach a tighter integration with schema validation☝️

i will find some links to other open source projects where i've seen this pattern.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Here's an example from the Dub project, where they define Zod schema and then declare types using z.infer

Copy link
Member Author

@johnclary johnclary Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another approach that i think is fairly common is to use a tool like graphql-code-generator to create your type declarations from your graphql API.

Here's a demo project that does this:

  • Here are the code-generated types
  • And here is a hook that uses the type

The idea is that you would re-run the code-gen scripts whenever you API changes.

You can see in that generated.ts file that it produces a lot of code, because it builds up the entire graphql API starting with the scalars. It's a pretty cool learning tool, and I like the example utility types like Maybe and Exact, but I don't love having all these typings in the code base when you're only using a subset of them. Not to mention that you really need to hope that code generator is maintained throughout the life of your own project 😵

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ this second example does not do runtime type checking at all. it just hopes that the type declarations match what the API returns.

Once we adopt typescript across our project, one of the most likely sources for errors is going to be situations where our API does not return what we're expecting it to (for example, because we forgot to add a field to a query or we changed a field's type from number to string).

The reason we would reach for Zod (or Yup, etc) would be to centralize the error handling in those cases. We can check types before data passes from our API to UI components, and throw helpful error messages when something goes wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i created cityofaustin/atd-data-tech#20024 for this and am going to do a little spike setting up a proof concept branch for how i think we want to structure this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is a branch/PR with a working setup for zod-driven typings:

sorting this out correctly and figuring out exactly what kind of tooling we want feels like the biggest open question for this app at the moment. I will update that PR with some more specific questions i have.

Once folks have a chance to catch up on this it will probably be worth scheduling a meeting to discuss :/

Copy link
Contributor

@mddilley mddilley Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next up, i plan to look at the zod PoC PR early next week. I know you mentioned in Slack that it may not be your favorite at the moment. I think a separate spike into the gql codegen could be cool like you suggested. Especially if we could find a way to be selective about the generated types and what ends up being stored in this codebase.

I guess we would also need to think about what updating types would look like too if we added another CRIS value in the crash table for example. 🤔

chiaberry and others added 30 commits January 6, 2025 15:47
Crash details: configure lookup tables for light condition, roadway part, and roadway system
…-counts

Add total record count to tables
Crash details: Ability to swap addresses
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants