usds / us-forms-system

Build React forms with JSON Schema and the U.S. Web Design System
Other
140 stars 34 forks source link

Move routing functionality to starter app #257

Open annekainicUSDS opened 6 years ago

annekainicUSDS commented 6 years ago

There have been a couple of requests for upgrading to react-router 4, or even making the routing of our app more configurable so that someone can use their own router.

How does routing interact with us-forms-system?

In starting to look into upgrading react-router, we decided to separately investigate the work involved in pulling out routing from the core library. There are several questions I think we need to answer in order to know what this should look like.

  1. What does the library look like without routing? Would it be usable without any form of routing?
  2. How does the core concept of pages and chapters work without a concept of routing?
  3. Would the navigation buttons still exist or would any concept of navigation also be pulled out of the library? How does this impact validation?
  4. Where does the routing functionality we currently have go? In the starter app, or in an entirely new repo (us-forms-system-router)?

Where is routing currently used in us-forms-system?

There are a few different places in the code where routing comes into play:

createRoutes

Main createRoutes function that generates the list of routes.

Things to think about:

withRouter() wrappers

We're using the withRouter() function to wrap several of our components. This gives the components access to props that give us control of the routing in those components.

Right now there are several components that are wrapped in withRouter(), but actually only 2 of them make use of it:

  1. <FormPage/>: the router prop is used to push the next or previous path based on if the user clicked the forward or backward button
  2. <SubmitController/>: the router prop is used to push the previous path based on if the user clicked the backward button

Things to think about:

  1. How we extract routing from these components largely depends on the answer to questions 2 and 3 at the top: how extracting routing impacts the concepts of pages and navigation. Should we just remove the navigation buttons from these two components entirely and somehow include that separately? Perhaps as wrapping components on the basic form page components?

Helper functions

Helper functions like getNextPagePath, getPreviousPagePath, and others used by createRoutes would need to be pulled out too.

Things that depend on a concept of a "route", but necessarily how to get there

There's some logic around breaking up parts of the form config based on the "route" (see FormPage), and there's some sense of it related to when validations are run and on what fields. But I don't think this stuff has to change in order for us to pull out routing.

annekainicUSDS commented 6 years ago

@dmethvin-gov would love to get your thoughts on the above! Also tagging @peterpme because I know you expressed interest in being involved in this work!

annekainicUSDS commented 6 years ago

My first pass at trying to answer some of the broader questions:

1. What does the library look like without routing? Would it be usable without any form of routing? The core library would be used for the logic around rendering form fields based on a config. It would handle all of the functionality we currently use around custom form fields and widgets, conditional logic, custom validation, and USWDS styled components. Nested objects in the form config would still signify separate sections or pages of questions.

2. How does the core concept of pages and chapters work without a concept of routing? I suppose it could still be separate pages and unique routes, but it would be up to a separate library to define how those routes are navigated to, whether that be through progress buttons at the bottom of each page to go forwards or backwards (like we do now), or through tabs or a side navigation to go to particular pages. We could provide that other library, or it could be something that a user of USFS would customize.

3. Would the navigation buttons still exist or would any concept of navigation also be pulled out of the library? How does this impact validation? I would think that navigation within the form would need to be pulled out.

4. Where does the routing functionality we currently have go? In the starter app, or in an entirely new repo (us-forms-system-router)? I think initially build it in the starter app, but eventually it might make sense to create a separate library for routing and navigation tools.

dmethvin-gov commented 6 years ago

@annekainicUSDS that looks about right to me! I think we should lean heavily on the starter app for now, and even create multiple apps if it makes sense to demonstrate different types of navigation or other features. We can always refactor useful parts to their own repos once we have a better idea of what they should do.

Although we use a router, we don't keep enough state to go to a "page" on a form. The router is making the address bar pretty but not functional for the random-access case, it's just handing the browser's forward/back button. For a small form, particularly a one-pager, I can imagine using USFS inside a Drupal or Wordpress site and not having it change the URL at all, just keeping the current state in JavaScript.

Even without a router, we'll need the getNext/Previous logic in order to determine the next or previous page of form. That's logic that requires specific knowledge about both the form config and the current form state (where the user may have added new pages).

I think the primary thing here is that even if every app using USFS ends up having a router, it does not follow that we should wire together the router and USFS behind the scenes in a way that apps have no control over. This Kent C. Dodds talk shows why embedding everything in a library that way can be a trap and makes a good case for the Inversion of Control pattern.

jbalboni commented 6 years ago

Maybe you have done more of this than I'm aware of, but I think you should figure out what you want this project to be and how you want to structure it before trying to pull out the router, which touches a lot of code. There's also a lot of code that doesn't directly touch react-router, but doesn't make sense outside of a multi page form application. I also think you would be encouraging bad single page applications to be built if you leave the concept of multiple pages and navigating between them without hooking that up in some way to a router (either a specific one or a configurable one) and breaking how history navigation is expected to work by users.

For example, there are three use cases I could see this library addressing, probably in multiple packages:

The first use case is a pretty clear-cut value for lots of people. And while I'm sure there are people who fall into the second use case, are they really better off with us-forms-system for that, or are they better off with the basic form fields in the first use case and something like Formik? The third use case was very valuable to vets.gov and I think the vets.gov experience is what makes people interested in us-forms-system so far.

Pulling things out without a clear conceptual idea of what this forms system is going to be is I think a risky move.

dmethvin-gov commented 6 years ago

At the moment, USFS is more of a complete application than a library. That's not unexpected since it has dependencies and baked-in assumptions about its use that were inherited from vets.gov.

To address more use cases we need to decouple things. Otherwise we're telling people they have to buy into a tightly coupled list of specific components and even specific versions of those components. I'm not sure why giving people more control over the router leads to risk, but I can see adoption risk if people don't have that control.

It still seems to me that the impact of this change is relatively small and beneficial, but let's all wait until the PR is ready and take a look at what it makes easier vs what gets harder.

jbalboni commented 6 years ago

I'm not saying you shouldn't give people control over the router. I agree that you should and that this library needs to be broken up or re-organized to be broadly usable. But there are a few ways to accomplish that and would result in very different PRs. So I'm advocating some up front thought about use cases, since routing is a core part of the value proposition that got this spun into its own project. I just don't want that to get pushed aside because of a versioning issue or conceptions about what libraries have to be.

dmethvin-gov commented 6 years ago

Here's an outline of the way I saw it working, there could definitely be other approaches so if you have some thoughts fire away. I talked with Anne about it this morning and she was going to do some more digging as well.

One question I had was about the showPagePerItem feature. It seems like that impacts all sorts of data structures and routes.

annekainicUSDS commented 6 years ago

Latest decision point on this: pull out navigation, header, footer, other wrapping elements to see what that looks like. Will push a PR up for discussion and review.

jbalboni commented 6 years ago

That seems like a reasonable approach toward the making the router configurable end of the spectrum (vs removing routing), though I haven't looked at all the router uses in a while. pagePerItem is complicated, but I don't think it will cause specific problems with that approach, aside from assuming a particular route template format.

dmethvin-gov commented 6 years ago

Sorry if my view wasn't stated very clearly above, but I always assumed our starter app would retain a router as an example of typical usage. The goal is to avoid having the library in this repo choose and require a router. This will result in removing the router dependency from the us-forms-system GitHub project we're in right now, so the term "removing routing" still applies from the perspective of this repo.

jcmeloni-usds commented 6 years ago

@dmethvin-gov +1

jcmeloni-usds commented 6 years ago

Per discussion, moving to Phase 2, milestone 2.0.0-alpha.1

annekainicUSDS commented 6 years ago

Where I left off with this work:

I was able to pull out all knowledge of routing from USFS and move it into the starter app, but not everything was working correctly. Issues I found were:

  1. Some logic in lower components that expected navigation buttons to be nested as children created problems now that those navigation buttons have been pulled out.
  2. When entering data into a field, the proper redux actions were firing to update the form data, but that new data for some reason disappeared from the field after the update to state was made. I suspect this has something to do with the fact that the starter app has no knowledge of form state, but I'm not positive; I wasn't able to successfully trace where the data was being lost.

I documented what I learned in comments in the 2 PRs I had working for this issue:

  1. USFS PR: https://github.com/usds/us-forms-system/pull/262
  2. Starer app PR: https://github.com/usds/us-forms-system-starter-app/pull/21