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

Render optimization #2749

Merged
merged 10 commits into from
Nov 10, 2020
Merged

Render optimization #2749

merged 10 commits into from
Nov 10, 2020

Conversation

jaredpalmer
Copy link
Owner

@jaredpalmer jaredpalmer commented Sep 17, 2020

This begins preparing Formik for concurrent mode.

  • Add unstable_useStrictField and unstable_StrictField to exports. They actually work like FastField and a useFastField (if it existed). I left FastField as is for now because it's not 1:1 with StrictField, although one could argue that it really is since state is stale in FastField's render props on other fields. Anyways, we can and need to debate naming and deprecations going forward.
  • Add a formik-codemod package in preparation for name and import changes
  • Clean up and dedupe handleChange and handleBlur internals once we figure out where things are going

Closes #1007, #450, #945, #2327

Proof of Concept: https://codesandbox.io/s/formik-use-context-selector-2-hso7i?file=/src/App.tsx

Basic custom field

Stays the same

// this is what useField does today.
// meta is safe for now, because it just contains error, touched, value and initials.
import { unstable_useStrictField as useField } from "formik";

export const CustomField = ({ label, name }) => {
  const [field, meta] = useField(name);
  return (
    <>
      <label>{label}</label>
      <input {...field} />
      {meta.error && meta.touched && <div>{meta.error}</div>}
    </>
  );
};

Custom Imperative handler of the same field

Bad (but, how it works today)

import { unstable_useStrictField as useField, useFormikContext } from "formik";

export const CustomField = ({ label, name }) => {
  const [field, meta] = useField(name);
  // This is bad news bears, but pretty common. Calling `useFormikContext`
  // means this component will re-render on every update to Formik,
  // even with this new PR.
  const formik = useFormikContext();
  const onChange = value => {
    formik.setFieldValue(name, value);
  };
  return (
    <>
      <label>{label}</label>
      <CustomInput {...field} onChange={onChange} />
      {meta.error && meta.touched && <div>{meta.error}</div>}
    </>
  );
};

Good

import { unstable_useStrictField as useField } from "formik";

export const CustomField = ({ label, name }) => {
  const [field, meta] = useField(name);
  // By making handleChange bound to the field and smarter about events
  // people will be able to avoid using setFieldValue(name, ...) and just do this:
  const doSomethingSpecial = () => {
    field.onChange("zop");
  };
  return (
    <>
      <label>{label}</label>
      <input {...field} />
      {meta.error && meta.touched && <div>{meta.error}</div>}
    </>
  );
};

Setting the value of another field or other Formik state

Bad

import { unstable_useStrictField as useField, useFormikContext } from "formik";

export const CustomField = ({ label, name }) => {
  const [field, meta] = useField(name);

  // This is bad news bears, but pretty common. Calling useFormikContext
  // means this will update on every key stroke / update to Formik,
  // even with this new PR.
  const {
    setFieldValue,
    values: { otherFieldName }
  } = useFormikContext();
  
  React.useEffect(() => {
    if (field.value === "jared") {
      setFieldValue("otherFieldName", "is cool");
    }
  }, [field.value, otherFieldName]);

  return (
    <>
      <label>{label}</label>
      <input {...field} />
      {meta.error && meta.touched && <div>{meta.error}</div>}
    </>
  );
};

Good

import { unstable_useStrictField as useField, useSetFieldValue } from "formik";

export const CustomField = ({ label, name }) => {
  const [field, meta] = useStrictField(name);
  // By using this new hook (which uses the new context selector under the hood),
  // this component now only re-renders when there are changes
  // from useField (instead of on every key stroke and update to Formik).
  const setOtherValue = useSetFieldValue("otherFieldName");

  React.useEffect(() => {
    if (field.value === "jared") {
      setOtherValue("is cool");
    }
  }, [field.value]);

  return (
    <>
      <label>{label}</label>
      <input {...field} />
      {meta.error && meta.touched && <div>{meta.error}</div>}
    </>
  );
};

@vercel
Copy link

vercel bot commented Sep 17, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/formium/formik/b12x0f7f9
✅ Preview: https://formik-git-use-context-selector.formium.vercel.app

@johnrom
Copy link
Collaborator

johnrom commented Sep 18, 2020

Why don't we expose all the stable methods from a single API hook like useFormikApi(), and expose the unstable values from something like useFormikState(). If someone needs both, they can do const [state, api] = useFormikState(); since adding the API isn't expensive, but adding state to API would be. @FredyC and I fleshed out a really nice API for it, and useFormikApi and useFormikState were his suggestions. Check out the discussion here

The API would support useContextSelector nicely I think.

@jaredpalmer
Copy link
Owner Author

We can have useFormikActions(). However, as mentioned, a big useFormikState() isn't performant. It will subscribe to all changes. With this PR, we can use selectors to limit updates. useFieldValue(firstName) will only update when that firstName value changes (not when firstName.touched or firstName.errors changes or when lastName.values changes). This solves the re-rerendering woes in the same way that recoil does: with selectors.

@jaredpalmer jaredpalmer added Component: Field Focus: Performance Enhancing Field and Form performance / rendering labels Sep 18, 2020
@jaredpalmer
Copy link
Owner Author

@johnrom you are correct that we can just have useFormikApi instead of a bunch of little helpers for setters. I still had my Recoil hat on. But since we don't have atoms to pass around this isn't necessary.

@sibelius
Copy link

we tested this PR and it is still rerendering everything

so the idea is to rebuild most of the hooks using unstable_ prefix?

@jaredpalmer
Copy link
Owner Author

Yeah i didn't implement any of the re-rerendering optimizations yet.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 24, 2020

Size Change: +4.88 kB (10%) ⚠️

Total Size: 45.9 kB

Filename Size Change
packages/formik/dist/formik.cjs.development.js 18.4 kB +1.95 kB (10%) ⚠️
packages/formik/dist/formik.cjs.production.min.js 8.1 kB +1.02 kB (12%) ⚠️
packages/formik/dist/formik.esm.js 18.3 kB +1.91 kB (10%) ⚠️
ℹ️ View Unchanged
Filename Size Change
packages/formik-native/dist/formik-native.cjs.development.js 306 B 0 B
packages/formik-native/dist/formik-native.cjs.production.min.js 242 B 0 B
packages/formik-native/dist/formik-native.esm.js 238 B 0 B
packages/formik-native/dist/index.js 150 B 0 B
packages/formik/dist/index.js 143 B 0 B

compressed-size-action

@@ -631,7 +631,7 @@ export function useFormik<Values extends FormikValues = FormikValues>({
if (!isString(eventOrTextValue)) {
// If we can, persist the event
// @see https://reactjs.org/docs/events.html#event-pooling
if (!!(eventOrTextValue as React.ChangeEvent<any>).persist) {
if ((eventOrTextValue as any).persist) {
Copy link
Collaborator

@johnrom johnrom Sep 25, 2020

Choose a reason for hiding this comment

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

This confuses me. eventOrTextValue is already string | React.ChangeEvent<any>, so if !isString(eventOrTextValue) then eventOrTextValue will automatically be inferred as React.ChangeEvent, which means there shouldn't be a cast here at all. Are we using an old typescript version? Or the wrong definition of isString which does not use type predicates?

-if ((eventOrTextValue as any).persist) {
+if (eventOrTextValue.persist) {

Copy link
Owner Author

Choose a reason for hiding this comment

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

Idk this didn’t have anything to do with this PR it just showed up in ts. reality is that the types might be lying (if we are in RN) and also that in react 17 this is a no op anyways.

@tooppaaa
Copy link

Hello, happy to help contributing to this PR with your guidance.

Please let me know if you need anything !

@johnrom
Copy link
Collaborator

johnrom commented Oct 30, 2020

With this PR, we can use selectors to limit updates. useFieldValue(firstName) will only update when that firstName value changes (not when firstName.touched or firstName.errors changes or when lastName.values changes).

I think we should consider dependent fields if we're going to take this approach. We could add an include= Field-level prop for dependent fields, like:

<Field name="fieldA" />
<Field name="fieldB" include={{ fieldA } => { fieldA }} validate={
  (value, dependentFields) => value * dependentFields.fieldA < 100
} />

Then this field's render and field-level validation can be triggered when either fieldA or fieldB changes.

@jaredpalmer
Copy link
Owner Author

With this PR, we can use selectors to limit updates. useFieldValue(firstName) will only update when that firstName value changes (not when firstName.touched or firstName.errors changes or when lastName.values changes).

I think we should consider dependent fields if we're going to take this approach. We could add an include= Field-level prop for dependent fields, like:

<Field name="fieldA" />
<Field name="fieldB" include={{ fieldA } => { fieldA }} validate={
  (value, dependentFields) => value * dependentFields.fieldA < 100
} />

Then this field's render and field-level validation can be triggered when either fieldA or fieldB changes.

@johnrom This is a good idea. Any thoughts on whether we should call this StrictField or FastField vs. break Field?

@johnrom
Copy link
Collaborator

johnrom commented Nov 5, 2020

I keep going back and forth on the API I suggested above. Seems like it could be done with useField and a custom prop. If we went this route, I'd prefer not to branch out into another component like FastField -- we've all seen the fallout of deprecating it. I feel like there should be one Field. I wonder if there is a way we can opt in to the new behavior at build time with a feature flag? This Context Selector business is very cool, but it's likely to be deprecated if the React team ever releases a solution which optimizes these renders via another method. For example, the input lag issue would probably be solved by Concurrent Mode.

We may want a way for users to opt into this behavior so long as they need it, and if it ever becomes the default that's great. But if it gets deprecated users should be able to opt into it for the next major version or two instead of having to rewrite their apps. Where we could put a feature flag is a whole different question. Webpack plugin? Formik prop?

Apparently I contribute more questions than answers, lol

@changeset-bot
Copy link

changeset-bot bot commented Nov 8, 2020

🦋 Changeset detected

Latest commit: dce5e8d

The changes in this PR will be included in the next version bump.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jaredpalmer
Copy link
Owner Author

@johnrom Technically speaking, this implements FastField and useFastField correctly. Instead of having to tell people in the fast field example, "don't get other form values from in here!" this just removes them entirely from the render props. I agree it would be nice to have one version of Field, and I think we can get there, but I don't think people want to rewrite their apps. Unfortunately most fields should just work, but obviously some will not.

A couple of options,

  • Make this FastField and useFastField, breaking old FastField (which was janky anyways). Tell people to start using FastField everywhere. Deprecate Field with warning?
  • Move existing Field and FastField to a new formik-compat package. Add a codemod that rewrites imports to LegacyField and LegacyFastField and field to LegacyField.
  • Explore feature flags?

@jaredpalmer jaredpalmer changed the title Migrate context to use-context-selector Render optimization Nov 9, 2020
@jaredpalmer jaredpalmer marked this pull request as ready for review November 10, 2020 18:13
@jaredpalmer jaredpalmer changed the base branch from master to next November 10, 2020 19:58
@jaredpalmer jaredpalmer merged commit 9502997 into next Nov 10, 2020
@jaredpalmer jaredpalmer deleted the use-context-selector branch November 10, 2020 19:59
@johnrom
Copy link
Collaborator

johnrom commented Nov 10, 2020

Codemods sound cool if webpack/babel plugin & feature flags are just a crazy idea. I've never looked at how to build them, just thought it would be a modern approach for deprecating commonly used functionality. .Net uses these kinds of flags a lot in the compilation process which allows them to support tons of new functionality while not breaking old code.

@filipkrayem
Copy link

Hey @jaredpalmer, I'm glad you've just finished this feature as I'm building a really huge form in Formik and this is needed a lot, since it's really laggy. Could you tell me roughly when can we expect this change released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Field Component: Formik Focus: Performance Enhancing Field and Form performance / rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimizing validation for FastFields
6 participants