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

Move routing functionality to starter app #257

Open
annekainicUSDS opened this issue Sep 6, 2018 · 13 comments
Open

Move routing functionality to starter app #257

annekainicUSDS opened this issue Sep 6, 2018 · 13 comments
Assignees
Labels
[practice] engineering Engineering related work
Milestone

Comments

@annekainicUSDS
Copy link
Contributor

annekainicUSDS commented Sep 6, 2018

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:

  • It would probably be beneficial if users had some control over this function, or at least would be able to overwrite it, like Vets.gov currently does. Then they could add their own logic, like if they assume Introduction and Confirmation pages, which components get rendered by default for each page, etc.
  • This could conceivably work for both react-router v3 and v4: for v3 we'd keep it as an object, and for v4 we'd convert it to outputting a hierarchy of React components. Not sure how this would work for entirely different routing libraries.

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 annekainicUSDS added the [practice] engineering Engineering related work label Sep 6, 2018
@annekainicUSDS annekainicUSDS self-assigned this Sep 6, 2018
@annekainicUSDS
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor

jbalboni commented Sep 7, 2018

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:

  • React form fields based on USWDS. I've built these several times in different projects now, it'd be nice to have a set of components that handle label and error rendering for you for a particular type, with no other dependencies than React. You could adapt the widgets and FieldTemplate components for this and they'd be usable for teams using other form libraries.
  • A basic form. Sort of what SchemaForm encompasses today, a way to build a simple form based on a config, that helps you with conditional and runs validation logic according to best practices. This is what @dmethvin-gov mentions with a form being inside another system.
  • A multi-page form. Essentially what vets.gov gets out of us-forms-system. This can be supported in different ways. If you could make the router configurable, you could have a core multi page helpers package and multiple packages for different widely used routers. Or just the core package and starter apps for different routers.

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.

  • At startup the app asks USFS for a list of the routes we want, which we know based on the formConfig. The app uses that to build its bigger list of routes using its preferred router, including routes that don't have any USFS on them like Introduction or Confirmation.
  • When the app goes to a particular route that is one that USFS knows about, USFS shows that page of the form based on a route-to-formPage mapping that it's built.
  • When the user presses the back/next button in side the form we need to tell the app to go to whatever route that is, e.g. the Next button onClick calls this.props.routeChange('user-address'). These just replace the existing router.push() calls currently being made. The route change will in turn change props sent to the React components and render a different page of the form.
  • When the form is submitted we need to tell the app there is nothing more for USFS to do and it should display whatever route it wants to now, e.g. USFS calls this.props.formSubmitted().

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

@annekainicUSDS
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

@dmethvin-gov +1

@jcmeloni-usds
Copy link
Contributor

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

@jcmeloni-usds jcmeloni-usds added this to the 2.0.0-alpha.1 milestone Sep 20, 2018
@annekainicUSDS
Copy link
Contributor Author

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: [WIP] Pull out routing #262
  2. Starer app PR: [WIP] Move routing here us-forms-system-starter-app#21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[practice] engineering Engineering related work
Projects
None yet
Development

No branches or pull requests

4 participants