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

refactor(core/presentation): Refactor FormField components using hooks #7148

Conversation

christopherthielen
Copy link
Contributor

In prep for some other work, I migrated the <FormField/> and <FormikFormField/> to use react hooks.

Here are some things I learned:

  • There are pitfalls with callbacks accessing stale values from closures. One possible way to address this is via useReducer because the reducer always receives fresh values.
  • Context usage is much lower friction than before.
    • const contextValue = useContext(ContextObject) - this can be used right inside the render, instead of wrapping a <Context.Consumer></> component around.
    • We have to expose the objects returned from React.createContext(), not just the Context.Producer and Context.Consumer objects.
  • useState to store simple local state
    • const [name, setName] = useState(initialName)
  • useEffect for side effects, which includes fetching data based on user interactions.
    • useEffect(() => api.checkIfNameExists(name).then(nameExists => setNameExists(nameExists)), [name]);
  • The deps array is very important, and makes it very easy way to react to changes in values.
  • If a component takes a callback, that API may instruct the caller to memoize the callback so === equality doesn't change and cause unnecessary work.
  • useMemo is an easy way to avoid expensive recalculations
    • const memoizedValue = useMemo(() => expensiveCalculation(inputString), [inputString]) - Is recomputed whenever inputString changes.

In general, this style of coding seems to lead to fewer if/then blocks, less imperative code, and more declarative code that defines the state of the component, and its output. I believe this will also replace much of the RxJS code in components, including the Observable.fromPromise(promise).takeUntil(this.destroy$).subscribe(result => {}) pattern.

Hooks will allow us to extract significant chunks of behavior, reducing code duplication. By encapsulating patterns as hooks, I think we can reduce friction for certain classes of problems (see how management of async data fetching seems simpler via useLatestPromise). However, hooks is a different mindset that will need to be grokked by deck developers. It's unfortunate that we are adding yet another paradigm to front end coding in deck (AngularJS, React Classes + lifecycle methods, React Hooks).

Copy link
Contributor

@maggieneterval maggieneterval left a comment

Choose a reason for hiding this comment

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

@christopherthielen thank you for the insightful PR description, I definitely do not yet grok the hook mindset so looking over this PR was very helpful!


public label = () => ifString(this.props.label);
const validate = useMemo(() => props.validate, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

In case anyone else looking at this PR had the same n00b question as me of the difference between the second argument to useMemo being an empty array vs. null, this Stack Overflow answer sums it up nicely.


public name = () => this.props.name;
const fieldLayoutFromContext = useContext(LayoutContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious what other Deck state you feel might be good candidate for storing on React context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wary of putting too much stuff in the context because it can make the answer to "where does this data come from?" harder to answer. There is a similar smell in AngularJS land -- you can put stuff higher up in the $scope tree and reference it down below, but tends to be an anti-pattern. It introduces undocumented, context sensitive dependencies.

Where I do think it makes sense is when you want to scope some setting to a portion of the DOM. For example, there may be multiple forms on the screen at the same time (each with different Layouts). Prop-drilling (things like layouts) all the way down to the leaf components would be explicit and easy to trace the data flow, but it would also be tedious and introduce excessive boilerplate to the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks as always for the thoughtful response! 😻

@christopherthielen christopherthielen merged commit d23ee1c into spinnaker:master Jun 28, 2019
@christopherthielen christopherthielen deleted the react-hooksify-form-field-components branch June 28, 2019 20:53
anotherchrisberry added a commit that referenced this pull request Jul 1, 2019
7d3b83a fix(core): correctly build permalinks for executions (#7167)
ccf080a fix(core/managed): add docs link to managed infra indicator (#7166)
d23ee1c refactor(core/presentation): Refactor FormField components using hooks (#7148)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants