I'm not sure I understand the purpose of this concept after reading this comment. Does Formik not support this use-case? I'm hesitant to think we're using it properly if we have to create all these types and a nesting system. Formik is so popular I feel like our use-case isn't unique.

It turns out we actually don't need this (at least not yet) - I thought it would be needed for the dynamic-form-parameters, but in this particular case it's not necessary.

Just for some extra context (the comment sure isn't very helpful) - there are actually are two use cases for forms that formik doesn't handle out of the box, that we needed for cdr/m:

• Re-usability of parts of forms (sub-forms/nested forms). For example - all Create/Edit forms have a Name and an Organization section. We had that duplicated across 2 strategies (Kubernetes, EC2) x 2 modes (Create and Edit) - and adding another one like Docker would end up with 6x of the exact same pieces of a form.
  • People ask for this, like in this Nested Forms issue - but formik's take is this doesn't belong in the core library and should be handled in userspace
  • ...and there are some user space solutions, like:
    • https://auto1.tech/the-concept-of-subforms-with-formik/
    • https://rajeshnaroth.medium.com/managing-nested-forms-gracefully-with-formik-a7ed35788653

A case where we used this is to have the Organization List for workspace providers be a reusable sub-form across all those providers - with the bonus that each of these sub-forms is easily testable.

• Dynamic Forms - we have use cases where parts of the form need to be changed based on other form fields. formik is great for static forms, but doesn't have any capabilities around dynamically changing parts of the form (and actually, having the nested/subforms is sort of helpful for this too).

An example of a use case where we need this in v1 is in the Create Workspace page - toggling between an EC2 provider vs a Kubernetes provider changes the Advanced Section:

It's possible to workaround not having dynamism in formik by having a 'fat interface' - having your form represent every possible state (like, having EC2/Kubernetes all on the form) - but it gets janky pretty fast, especially around validation. Before we had that subform - we'd have bugs where Kubernetes validation would run when EC2 provider is selected.

I put a lot more context here in introducing it for the EC2 / Kubernetes provider: https://github.com/coder/m/pull/10250

But anyway our here forms aren't crazy enough yet to need anything like this - so I'll keep it simple and remove for now! If we re-introduce, that means there'll be a clear use case for it.

If we keep our forms simple, we might not need it at all - this may be an extra benefit of the Projects workflow - that we don't need fancy dynamic forms 😎

Ah, I tried to remove it but unfortunately it turns out it is necessary to give strong-typing to our forms.

Our utilities - like FormTextField - depend on those utils to implement type-safety via typeof:

export interface FormFieldProps<T> {
  /**
   * form is a reference to a form or subform and is used to compute common
   * states such as error and helper text
   */
  form: FormikLike<T>
  /**
   * formFieldName is a field name associated with the form schema.
   */
  formFieldName: keyof T
}

(in addition to the nesting and dynamic aspects above). I think it'll be simplest to preserve these utilities so it's easy to migrate our other form components built on top of it. Hopefully that's OK with you!

The alternative is we can remove formik, and change all those keyof T fields (which provide the extra layer of type safety) with string (which is no longer type-safe, but will compile), and modify all the Form* components in that way as we migrate them from v1 -> v2

Do we believe it's safe to say all of our Form instances will follow a similar format? I have lots of hesitancy over a top-level form component. If we even have a single form that doesn't consume it, this top-level component abstraction has broken.

I'm not sure of an alternative though, so maybe this is required!

I think this is a good point for this component - it's only used in one places so I can bring it inline.

I've abstracted into this general page format before, and I've generally regretted it.

Could we put the styles.header directly on the Navbar component? And same for the footer? Then for root, put those in _app.tsx. Each page can put <Navbar /> and <Footer /> in itself. It's a bit of duplication, but I think that's probably fine!

This comment is a bit confusing, so please ping me if it doesn't make sense!

IMO, I think if we pick the right abstraction - these sort of components can reduce cognitive load when thinking about components.

Like, from my point-of-view:

export const ProjectsPage: React.FC = () => {
<AppPage>
  {...stuff to render projects...}
</AppPage>
}

is simpler to reason about than:

export const ProjectsPage: React.FC = () => {
<AuthenticatedRouter>
  <div-with-proper-fullscreen-styles>
  <Navbar>
  {...stuff to render projects...}
  </Footer>
  </div>
</AuthenticatedRouter>
}

In the first case - I can really focus on the 'guts' of the ProjectsPage, whereas in the second, I have to start think about other details - what are the right styles for the top-level component? Does the body need a wrapper div, or can it be at the same level as Navbar / Footer.

This composability of React is part of the power of it - there's the Thinking in React section in the React documentation that talks about some guidelines for thinking in components / breaking things up into components. Generally start with simple components and compose them into more intricate ones (but of course there's a lot of judgement in it).

I've abstracted into this general page format before, and I've generally regretted it.

I've seen some cases too like this - the flip-side is if you pick the wrong abstraction, and the user not only has to think about the abstraction but then twist it to fit their use-case - it ends up increasing cognitive load instead of decreasing it. I think an example of a case we hit in v1 was in the PaperForms - I'm not sure if the product evolved from the original design, I imagine it was very clean to start, but down the road we ended up in a place where the abstraction just didn't quite fit in any of our forms, and we'd have to introduce extra complexity just to make it work. Is that the sort of the thing you've ran into as well, or are you thinking about a different case?

The key to avoiding the case I called out above is to be diligent in monitoring the abstractions and making the right decisions in when to change / refactor and avoid making components too general. Sometimes by avoiding duplication we end up with components that try and handle everything, but don't handle anything cleanly! It feels like an art though sometimes more than a science...

My personal preference would be to keep this as <AppPage /> because I think it'll make my life easier as I add more pages - but if you feel strongly about decomposing it, we can do that and revisit down the road as we have more use cases to inventory across Login / Projects / Create / etc. Ultimately I want you to be happy with what we ship here 😄

A wonderfully elegant explanation of the rationale. I agree with your perspective, but I'm concerned about the ambiguity of an <AppPage /> specifically. Maybe the name rubs me the wrong way.

In my imagination, we'd structure things like:

UserContext that's added in _app.tsx. Every new page load the user would be fetched again. To consume the user context, a parameter is passed in whether to redirect or not. See:
https://github.com/coder/link/blob/main/site/src/contexts/UserContext.tsx#L20-L32

In each page, we'd consume the useUser() function. For authenticated pages, we'd have useUser(true) to redirect to login on failure.

Most of the page logic would be in the pages/projects.tsx page themselves, removing the need to abstract the bulk from _app.tsx -> pages/*.tsx -> components/*.tsx.

If we can accomplish a similar level of elegance with <AppPage />, I'm game for it. Better I just share my objective haha!