Why is this called PasswordField, but the other named FormTextField?

This is brought in mostly as-is from v1, but there's actually a has-a relationship here.

FormTextField contains either:

• TextField
• PasswordField

In the v1 coder/m codebase - we used the prefix Form* to denote that the controller is meant to be bound to a formik form. That means that it exposes a couple of properties:

• form: a formik form
• formFieldName: the field name to bind too

With that, the Form* components handle binding to Formik:

• onChange (calls into formik)
• onBlur (calls into formik)
• error (sets appropriate property on control based on form state + field)
• helperText (set to error text if there is an error, otherwise helper text if provided)
• id (set to form field name)

So essentially FormTextField is a convenience component that helps bind to formik - instead of having to bind to 5 different props to get proper form behavior (and potentially doing it inconsistently) we only have to bind to two props - form and formFieldName, and get common behavior across our forms.

And it happens that FormTextField can either use a TextField or PasswordField under the hood, depending on the isPassword prop. TextField and PasswordField don't know anything at all about formik - they're just 'dumb' input components.

In the v1/coder/m codebase, we actually have a bunch of these Form* components, including:

• FormCheckbox
• FormRadioGroup
• FormField
  ...and we've talked about a few others, like FormToggleSwitch

Hope that helps clarify - let me know if you have other questions

Ahh this is very helpful context. Thanks Bryan!

I know we didn't in the other repo, but it might be wise to consider a default export idiom for this codebase.

CoderIcon.tsx should default export CoderIcon from my POV.

This is actually kind of contentious in the TypeScript world - there's a couple of quirks with default exports.

• The refactoring tools in VSCode / TypeScript language server don't work as well with default exports
• In some compilation modes, the default export is kind of awkward (it literally exports a named module 'export', which can cause problems with some tools). NextJS handles this OK, but if we use a tool with a Babel pipeline like storybook, we can sometimes run into weird issues.

There's a bit more info in these links:

• https://stackoverflow.com/questions/33305954/typescript-export-vs-default-export
• https://basarat.gitbook.io/typescript/main-1/defaultisbad

That being said, it seems to work fine for our pages/ 😄 And I think the icons is a reasonable use-case for this, I agree with you conceptually that it makes sense for CoderIcon.tsx. We can try it out if you want.

With the context you provided here, I'll trust the direction you're taking here! Let's leave it as-is for now, everything seems to point away from the defaults hahah

Should we do this in _app.tsx instead? Seems quite standard.

As it is currently implemented - this is a case where it actually wouldn't make sense to include this in _app.tsx - if we did, the /login route would get in an infinite loop. (The _app.tsx would fetch the current user, fail, redirect to login.... which would render _app.tsx first, fail, redirect to login... forever 😄 )

But I think this an example of a case where something like an <AppPage /> can help us (from the comment in the other PR here: #33 (comment)) - every <AppPage /> should include this.

However, if we removed the login.tsx, and just implemented that full flow in _app.tsx - it could work. My concern is that the _app.tsx can grow really fast and it could also add complexity when we add other login flows besides built-in auth (ie, OIDC)

This might be a good opportunity to introduce SWC. It gives us a lot of beeeeautiful inherit benefits.

Primarily in periodically refreshing to ensure state is accurate with the backend.

Sure, I'm open to exploring it!

Are you OK if we track this outside of the PR? And do you think it is necessary for black triangle, or something we can defer until after?

Opened this issue to track, and I marked it as a MVP for the time being: #43 but let me know if its something you'd prefer to do in this PR (or outside the PR, but for black triangle, and I'll update the milestone)

Well, the reason I recommend we use it is actually for simplicity. Although there's little familiarity, I'm not sure there needs to be: https://github.com/coder/link/blob/main/site/src/contexts/UserContext.tsx#L35

Just makes it really simple, and removes the requirement of abstracting a fetch at all.

This is up to you as to whether we use it or not. It does handle all the errors n' such nicely though, so could be cool!

Cool, I'll take a look as part of this PR then and try it out! Thanks for the link to the usage in link - helpful

I tried it out and it actually makes things more complicated (for login, at least). It isn't really meant to POST or take body arguments out of the box.

Here's a GH issue describing setting up a login flow:
vercel/swr#553 (comment)

I'll revisit for some of the other places, though, where might be able to take better advantage of the error handling / refresh behavior.

Ahh I mean mostly for fetching. Much less useful for any other type of request.