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

v3 #2846

Closed
wants to merge 79 commits into from
Closed

v3 #2846

wants to merge 79 commits into from

Conversation

jaredpalmer
Copy link
Owner

@jaredpalmer jaredpalmer commented Oct 27, 2020

  • All hooks that return an initial thing return a single variable.
  • useIsDirty, useIsSubmitting, and useIsValid return booleans
  • All other hooks that return state also return updaters (e.g. const [values, setValues] = useValues())
  • Hooks that are name useSetXXXX just return the setter (and have no perf overhead)

Todo:

  • useFieldInitialValue
  • useFieldInitialTouched
  • useFieldInitialError
  • useInitialValues
  • useInitialErrors
  • useInitialTouched
  • useInitialStatus
  • useFieldTouched
  • useFieldValue
  • useFieldError
  • useSetFieldValue
  • useSetFieldTouched
  • useSetFieldError
  • useValues
  • useSetValue
  • useSetErrors
  • useErrors
  • useTouched
  • useSetTouched
  • useStatus
  • useSetStatus
  • useSubmitForm
  • useResetForm
  • useIsSubmitting
  • useIsValid
  • useIsDirty
  • useValidateForm
  • useValidateField
  • useActions ??? (bag of all methods)?
  • Provide way to call validateForm and validateField with hooks
  • Reimplement Form, ErrorMessage, and Field
  • Fix typescript types of Field and FastField render props
  • Docs

@jaredpalmer jaredpalmer requested a review from johnrom as a code owner October 27, 2020 18:38
@changeset-bot
Copy link

changeset-bot bot commented Oct 27, 2020

🦋 Changeset detected

Latest commit: 1c36c1c

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

@vercel
Copy link

vercel bot commented Oct 27, 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/160by3vfz
✅ Preview: https://formik-git-next.formium.vercel.app

@jaredpalmer jaredpalmer marked this pull request as draft October 27, 2020 18:38
@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2020

Size Change: +3.55 kB (8%) 🔍

Total Size: 43.3 kB

Filename Size Change
packages/formik/dist/formik.cjs.development.js 17.4 kB +1.48 kB (8%) 🔍
packages/formik/dist/formik.cjs.production.min.js 7.66 kB +705 B (9%) 🔍
packages/formik/dist/formik.esm.js 17.1 kB +1.36 kB (7%) 🔍
ℹ️ 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

@johnrom
Copy link
Collaborator

johnrom commented Jan 20, 2021

@jaredpalmer I think the lack of what I will call "dynamic slicing" is going to be a problem as the Formik API continues to evolve. The basic issue is this: You need to know what hooks need to be called in order to build a component with these useContextSelector-based hooks. You must be able to write a component which selects specific bits of Formik's state. You can't subscribe to a "random cross-section". And you cannot change those hooks based on dynamic props. If you need a random cross-section, you'll have to subscribe to all of Formik's state and select it yourself, leading back to where we are today.

A specific API which I'm thinking would be impossible with this is one I've been thinking of for a while:

<Field 
  type="number"
  name="width" 
  include={fields => fields.height} 
  validate={(value, height) => value * height.value > 100 
    ? "Max of 100 square feet."
    : ""
  }
/>

I believe my subscription based approach described here would enable that API in a performant way.

@jaredpalmer
Copy link
Owner Author

Why wouldn't useContextSelector be able to handle this (assuming you memoized the selector)?

@johnrom
Copy link
Collaborator

johnrom commented Jan 21, 2021

  1. From what I understand, useContextSelector is a pretty simple subscription which is limited to using Object.is comparison, which means if the selector returns an object, the object will never match, and the context will always update. Anytime values, errors, or touched are updated, all of the Fields would in theory update due to a new object created by useField(). However, your PR gets around this limitation by breaking useField into separate micro-hooks, like useFieldValue, useFieldTouched, etc. This optimization cannot be used for dynamic slices of Formik's state, unlike they can with a custom subscription. My Subscription PR allows a user to pass a comparer like Object.is (default), but could also use _.isEqual or any other simple comparison callback. useField would now consist only of one subscription.

  2. Due to using micro-hooks, in this PR every use of <Field /> uses 18 separate subscriptions to the useContextSelector provider, as well as other subscriptions in other places to access the API, which isn't necessary in my PR because I've made sure the API itself is stable (except from some config props, like validateOnChange, but that could probably be improved if we can agree to this state separation). You can check all the subscriptions here for a simple form with 3 fields. Note there are (50) total listeners to useContextSelector.

image

Meanwhile, in my PR, each Field only uses one subscription, as well as a normal Context subscription to access the API, which almost never updates due to stable references (once again, we can improve). Note there are 3 total subscribers, with 1 dispatch effect each.

image

And if multiple fields were all listening to the same selector + args + comparer combo, for example, selectFieldMetaByName, "firstName", _.isEqual or like selectIsSubmitting, Object.is, there would only be one subscription checking for equality, and when it is determined to have change, it can simply trigger all three listeners with the same value instead of checking each individually. Note this is 1 total subscriber, with 3 dispatch effects. I don't know how useful this is in reality, but somebody in one of the issues talked me into it 🤣.

image

In the 500 input performance test, this PR has 7502 listeners to Context, while @formik/reducer-refs has 500. I feel a performance difference when holding down a key in this test, on my PC. In this pr, while holding down a key my PC will never render a new value until I let go. On @formik/reducer-refs, it will print out about every 6 or 7th key regularly. That's about as scientific as I've bothered to get lol.

@dai-shi
Copy link

dai-shi commented Jan 25, 2021

For reference: dai-shi/use-context-selector#19 (comment)

@johnrom
Copy link
Collaborator

johnrom commented Jan 25, 2021

@dai-shi custom equality is only one difference between my implementation and useContextSelector.

  1. does useContextSelector provide direct access outside of Context?

not using the context (except to access the API) means you can use the internal values wherever you have access to the API, with my PR you can do something like:

const formik = useFormik(...props);
// you can subscribe to the "mutable source" without a context, much like you will be able to after useMutableSource lands
const formikSlice = useFormikStateInternal(formik, selectIsSubmitting, Object.is);

Or you could even pass a ref to formik and subscribe to it from any context you want (well, in theory, haven't tried).

  1. my subscriber provides the underlying object via getState() which is guaranteed to always have the latest value, without waiting for React commits. is that available in useContextSelector?

this particular latest value guarantee is necessary due to the fact that in many cases form state is not a function of React effects, but a function of user input which does not wait for effects to complete.

onSubmit={async (values, api) => {
  const result = await checkApiThing();
  // make sure these form values aren't stale
  const stillRelevant = isEqual(values, api.getValues());
  
  if (result.success && stillRelevant) {
    doImportantStuff(values);
  }
}}

@dai-shi
Copy link

dai-shi commented Jan 25, 2021

@johnrom sorry, i'm not familiar with context of the discussion here in formik.
I'm a bit confused. It sounds like the two points you are asking are not like React state and React context.

useContextSelector(ctx, selector) is just like selector(useContext(ctx)) semantically, and only adds optimization. Like you can only read it in render. So, I guess answers to the questions are both "no"s?

@johnrom
Copy link
Collaborator

johnrom commented Jan 25, 2021

@dai-shi thanks for your response. I'm familiar with the power of useContextSelector in optimizing context and suggested its use for Formik many months ago now. But after a lot of thinking, I have come to the conclusion that Formik should avoid using Context except to pass down its API and instead provide state access via direct and internally controlled subscriptions. There are plenty of packages that do this out of the box, like useSubscription, redux, etc, but the implementation in my PR is specifically crafted to handle the large number of similar subscriptions and updates that Formik generates by having dozens or more components subscribing to reusable cross-sections of state.

@dai-shi
Copy link

dai-shi commented Jan 25, 2021

Got it. Yeah, form store makes sense if that's the requirement. (I mean I know both needs. like, some of my libs are for react state and some are for module state.) It's just a bit unfortunate..

@johnrom
Copy link
Collaborator

johnrom commented Jan 25, 2021

@dai-shi would you be interested in reviewing my subscription implementation? I wrote the subscription part completely independent of Formik's general API and it is suitable to be broken into a separate package. There is overlap between this and useContextSelector, and I wonder if they can work together as a method to subscribe to Context.

The two files related to subscriptions are here:

subscription-helpers.ts
useSubscriptions

@jaredpalmer
Copy link
Owner Author

Is there a material performance difference between sharing a subscription and creating new ones with useContextSelect for forms with < 100 fields? Does this actually matter to end users?

@johnrom
Copy link
Collaborator

johnrom commented Jan 25, 2021

I didn't set out to optimize the subscription mechanism per se when I created my PR. I set out to include a subscription API that can allow us to:

  1. use custom equality comparisons to prevent Field re-renders
  2. dynamically slice formik state
  3. get the current state at any time (getState()), and
  4. provide access to the subscriptions without a context at all, as described here.

I couldn't find anything that fit the bill (including useMutableSource), so in order to proof that, I had to write a new subscription mechanism. Could definitely ship with useContextSelector level of performance (as long as common subscriptions don't use deep equality), but performance is not my first goal here.

@dai-shi
Copy link

dai-shi commented Jan 26, 2021

The two files related to subscriptions are here:

subscription-helpers.ts
useSubscriptions

It's more complicated than expected. Maybe because it's tree structure with subscriptions to avoid duplicated invocation of selectors?
I didn't go details, but it's the store outside React. Looks close to zustand, another library I maintain.
Honestly, I don't see any fundamental issues with this approach in Legacy Mode.
To prepare for Concurrent Mode, you want to consider the migration path to useMutableSource. (It's still questionable if it's really necessary, depending on use cases and requirements.)

@github-actions
Copy link
Contributor

This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

@github-actions github-actions bot added the stale label Feb 26, 2021
@dantman
Copy link

dantman commented Feb 26, 2021

The API appears to be missing useIsValidating.

@github-actions github-actions bot removed the stale label Feb 27, 2021
@johnrom johnrom mentioned this pull request Mar 3, 2021
@johnrom johnrom mentioned this pull request Mar 15, 2021
1 task
@johnrom johnrom added this to the v3.0 milestone Mar 15, 2021
@dantman
Copy link

dantman commented Mar 26, 2021

The various use* hooks need explicit return values, especially the ones that use tuples.

Take useValues for example:

export function useValues<Values>() {
  const state = useFormikContextSelector<
    Values,
    FormikContextType<Values>['values']
  >(ctx => ctx.values);
  const update = useFormikContextSelector<
    Values,
    FormikContextType<Values>['setValues']
  >(ctx => ctx.setValues);
  return [state, update];
}

The type that TypeScript infers is (Values | ((values: SetStateAction<Values>, shouldValidate?: boolean | undefined) => void))[]. What we actually want is [Values, ((values: SetStateAction<Values>, shouldValidate?: boolean | undefined) => void)].

@github-actions github-actions bot added the stale label Apr 25, 2021
Repository owner deleted a comment from github-actions bot Apr 25, 2021
@jaredpalmer jaredpalmer marked this pull request as ready for review April 25, 2021 12:02
@jaredpalmer jaredpalmer removed the stale label Apr 25, 2021
@github-actions
Copy link
Contributor

This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

@github-actions github-actions bot added the stale label May 26, 2021
@github-actions github-actions bot closed this Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants