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

Use FormikApi + FormikState Subscriptions #3089

Closed
wants to merge 30 commits into from

Conversation

johnrom
Copy link
Collaborator

@johnrom johnrom commented Mar 11, 2021

This pivots Formik away from using Context for anything but the API itself, and exposes a formik.useState() implementation to subscribe to Formik State via a selector and comparer when you need to do specialized equality checks. Implemented with use-subscription.

Upgraded Dev Environment

The new Development environment is awesome. Try it out. VS Code will now run your tests automatically with the Jest extension. Running npm run start:app, or just Launching from VS Code, will now run the app/ folder aliased directly to Formik's source at localhost:3000, so you can test and develop Formik directly through /App. You can also launch Chrome against it to attach breakpoints to Formik.

Added Ref State.
Added useSelectorComparer.
Starting to build subscriptions.
use-subscriptions might not work in React 17? Seems returning the previous value doesn't bail out the render.
Sync up formik-native and formik for v3.
@changeset-bot
Copy link

changeset-bot bot commented Mar 11, 2021

🦋 Changeset detected

Latest commit: f3bd303

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

This PR includes changesets to release 3 packages
Name Type
formik-native Major
formik Major
app Major

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 Mar 11, 2021

@johnrom is attempting to deploy a commit to the Formium Team on Vercel.

A member of the Team first needs to authorize it.

initialValues: state.initialValues,
});

export const useFormikApiComputedState = <Values = any>(formikApi: FormikApi<Values>) => {
Copy link
Owner

Choose a reason for hiding this comment

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

consider removing? or replacing with just useIsValid useIsDirty

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed.

This comment was marked as outdated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved computed state to api.useComputedState() so it is accessible from anywhere, and only expose the hooks useIsValid and useIsDirty.


export const selectFullState = <T>(value: T) => value;

export const useFullFormikState = <Values>(
Copy link
Owner

Choose a reason for hiding this comment

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

full, computed, api, state, selectFull, need to reduce the surface area.

Copy link
Collaborator Author

@johnrom johnrom Mar 15, 2021

Choose a reason for hiding this comment

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

  • useFormikApi is needed, as it gets the Context, which is now only the API
  • useFormikState is an alias to (selector, comparer) => useFormikApi().useState(selector, comparer), which seemed a little tedious to make users use. However, we have to expose that for any of this to work, so it's up to you whether we keep this hook or make users use useFormikApi().useState()
  • useFormikApiState(api, ...etc) can probably go, since it is just api.useState(selector, comparer);
  • selectFullState is not something we should expose, it's just something we use internally instead of memoizing a new selector. I can go through and mark stuff like this @internal.
  • useFullFormikState is used by backwards compatible useFormikContext which users expect to load the legacy "formikBag". It's also used internally to subscribe to the full state in the case of Formik and Field render / child fns which do not have access to hooks to get Formik's state. it doesn't need to be exposed -- I can mark it @internal

Copy link
Owner

Choose a reason for hiding this comment

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

Can useFormikState become useFormikStateSelector() or just useFormikSelector()?

I would also like to suggest changing useFormikApi() to useFormikActions() or useFormikMethods(). Mentally, easier to think about state or actions/methods versus state or api I think. api could be anything and it isn’t clear whether or it holds state.

Copy link
Owner

Choose a reason for hiding this comment

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

Edit: I now see that these mean different things and my prior comment is wrong.

I still think API is a meaningless word and would like to see it changed. useFormikApiState vs. useFormikApi vs. useFormik vs. useFormikState isn’t intuitive for end users.

Copy link
Collaborator Author

@johnrom johnrom Mar 17, 2021

Choose a reason for hiding this comment

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

Formik API is the only word here that makes sense to me.

  • A getter is not an action
  • A method is a function belonging to an object in OOP, which isn't really applicable to functional programming like Formik whereas it might apply more in other form APIs since they use classes under the hood.
  • The type returned by useFormik is FormikApi and the type exposed by Context is also FormikApi, so useFormikApi seems to be the best naming convention for the context since we can't use useFormikContext.

If you do not believe that useFormik returns an API, that is a whole different story. I don't think it returns Methods, or Actions. Maybe Helpers?? But we already draw a distinction in TypeScript between Helpers, Handlers, and Config.

Let's decide what the TypeScript name for the return value of useFormik is, and we'll change the name of useFormikApi to match. Whatever it is, the Context holds that value so the names should match.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it'll be super clear that "API" is the right word for the API that Formik provides if we document it separately from the various components and hooks that we provide, and separately from the State.

A person who simply uses useFormik() and builds everything else from scratch is only using the Formik API, and we should document that separately. The current documentation uses API Reference to mean "all the things" whereas the API Formik provides is just what is returned by useFormik and every other component or hook we provide is a shortcut for using that API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can always break backwards compatibility and switch useFormikContext to just have the API, and then it'll be useFormikContext and useFormikState.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did the above, switched back to useFormikContext and useFormikState, abandoning backwards compatibility for clarity.

@@ -103,6 +103,7 @@ describe('withFormik()', () => {
setSubmitting: expect.any(Function),
setTouched: expect.any(Function),
setValues: expect.any(Function),
isFormValid: expect.any(Function),
Copy link
Owner

Choose a reason for hiding this comment

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

isValid?

Copy link
Collaborator Author

@johnrom johnrom Mar 15, 2021

Choose a reason for hiding this comment

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

isFormValid is a function passed by Formik API which is needed to calculate whether the form is valid. Otherwise, we need to pass a bunch of config props from Formik through the API, and if a user doesn't memoize them we lose all of the performance gains.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed isFormValid in favor of exposing formikApi.useComputedState().

@johnrom johnrom mentioned this pull request Mar 15, 2021
1 task
@johnrom johnrom added this to the v3.0 milestone Mar 15, 2021
@johnrom
Copy link
Collaborator Author

johnrom commented Mar 17, 2021

This currently passes the following props through the API which I think we should remove, because they are likely to cause re-renders if a user doesn't optimize them.

{
  validationSchema?: any | (() => any);
  validate?: (values: Values) => void | object | ValidateFn<Values>;
}

@johnrom
Copy link
Collaborator Author

johnrom commented Mar 18, 2021

I found a small issue with this method. If we dispatch two values in a row, it's theoretically possible to skip a dispatch using use-subscription because it comes directly from getState() instead of state. This is fine for the render because it'll always become consistent with the latest info, but it's not OK if a dev needed two discrete effects to be triggered. This didn't happen in my original subscription implementation. I'll write a test for this scenario.

Proposed solution is to add a useEventCallback version of getState which can get the state snapshot returned from React.useState(), and use that in the selector for use-subscription.

Ignore the above, as this is just how batching works in React. I've been thinking about this for too long. 😂

I'm going to start adding some of the app/ code from #2846 and #2931 over the next week and do some more testing as well.

johnrom and others added 3 commits March 18, 2021 18:09
…ormikReducerState + FormikComputedState.

Add Fixtures and Tutorial code to /app.
Consolidate State and Add Tutorial + Fixtures.
@johnrom
Copy link
Collaborator Author

johnrom commented Mar 22, 2021

I figured out how to remove useComputedState entirely. Now useState() will return FormikState, which is FormikReducerState + FormikComputedState, simplifying the API dramatically.

@johnrom
Copy link
Collaborator Author

johnrom commented Mar 23, 2021

@jaredpalmer this is ready for review and I believe I've resolved all the items above. I did the following:

  • removed all State from FormikContext and switched back to calling it FormikContext instead of FormikApi
  • combined computed state into api.useState / useFormikState, removing all the confusion around computed vs reducer state
  • removed isFormValid function as isValid is computed during getState()

You can test the branch locally by pulling the branch in VS Code, running yarn to link the workspaces, and running the run npm dev task. It will boot up /app folder connected directly to /packages/formik/src.

@johnrom johnrom changed the base branch from master to milestone/v3 March 23, 2021 20:44
Copy link
Owner

@jaredpalmer jaredpalmer left a comment

Choose a reason for hiding this comment

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

Almost there...

Selector,
useOptimizedSelector,
} from './hooks/useOptimizedSelector';
import { unstable_batchedUpdates } from 'react-dom';
Copy link
Owner

Choose a reason for hiding this comment

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

how does this work with react native?

Copy link
Collaborator Author

@johnrom johnrom Apr 10, 2021

Choose a reason for hiding this comment

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

I'll have to test it out, but I think you're right this needs a different import for react native. I've never actually used React Native (except to play with the default expo starter), but maybe we should have an expo version of app/ for testing?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't need to manually batch updates here anymore as I moved this into an effect. I'll remove and test.

if (__DEV__) {
// eslint-disable-next-line react-hooks/rules-of-hooks
React.useEffect(() => {
invariant(
typeof isInitialValid === 'undefined',
typeof props.isInitialValid === 'undefined',
Copy link
Owner

Choose a reason for hiding this comment

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

Should we actually break the rules of hooks here? (i believe the rationale is that we only ever want to show this error message once) If so, let's add a comment about it or fix this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I didn't make any changes to this except to update the variable, but I believe you wrote it so that it would only print the warning once in development on the first use.

I would create a separate issue for this since it's unrelated to this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created #3145 for this, as I wasn't involved in the DEV checks.

/**
* This is the true test of spacetime. Every method
* Formik uses must carefully consider whether it
* needs to use the ref or the render snapshot.
Copy link
Owner

Choose a reason for hiding this comment

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

Elaborate more here in comments for outside contributors as to why we need this optimization. Include mental model for deciding whether to use ref and or snapshot.

Copy link
Collaborator Author

@johnrom johnrom Apr 11, 2021

Choose a reason for hiding this comment

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

I currently left the code functioning how it does currently even though I created the ability to "fast-forward" via refs. But I need to go and revisit exactly what each bit of Formik uses, and whether it currently uses the right one. I'm pretty sure the answer is always use getState() internally, except for code used inside useState.

@@ -334,53 +398,54 @@ export function useFormik<Values extends FormikValues = FormikValues>({
}
);

React.useEffect(() => {
const performValidationOnMount = useEventCallback(() => {
Copy link
Owner

Choose a reason for hiding this comment

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

executeValidationOnMount for consistency?

isMounted.current === true &&
!isEqual(state.initialTouched, nextInitialTouchedProp)
) {
const touched = nextInitialTouchedProp || emptyTouched;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const touched = nextInitialTouchedProp || emptyTouched;
const touched = nextInitialTouchedProp ?? emptyTouched;

import { isShallowEqual } from '../utils';

/**
* Returns @see FieldMetaProps<Value>
Copy link
Owner

Choose a reason for hiding this comment

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

Improve comment. This will be shown in intellisense.

import { FormikApi, FormikReducerState } from '../types';

/**
* @see {@link FormikApi['useState']} for info on using Formik's State.
Copy link
Owner

Choose a reason for hiding this comment

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

Improve comment

export type Comparer<Return> = (prev: Return, next: Return) => boolean;

const UNINITIALIZED_VALUE = Symbol();

Copy link
Owner

Choose a reason for hiding this comment

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

Add comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've moved this package to its own unit tested npm package in another PR. see #3124.

@@ -1,3 +1,5 @@
export * from './hooks/useFormikState';
export * from './hooks/hooks';
Copy link
Owner

Choose a reason for hiding this comment

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

We are exporting useOptimizedSelector?

Copy link
Collaborator Author

@johnrom johnrom Apr 10, 2021

Choose a reason for hiding this comment

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

In another PR under this milestone, I move useOptimizedSelector to a separate package with unit testing etc. You should check that out before I modify this.

Also, I don't think I exported this but I'll double check.

Copy link
Collaborator Author

@johnrom johnrom Apr 12, 2021

Choose a reason for hiding this comment

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

It is not exported publicly by index.tsx

export type SetSubmittingFn = (isSubmitting: boolean) => void;

export type SetTouchedFn<Values extends FormikValues> = (
touched: import('./types').FormikTouched<Values>,
Copy link
Owner

Choose a reason for hiding this comment

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

why is this import() here? is this importing itself? that works???

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think vs code autocompleted this... And somehow it works I guess?! Lol

@johnrom
Copy link
Collaborator Author

johnrom commented Apr 12, 2021

@jaredpalmer I simplified the mental model here by enforcing a boundary between the state used under the hood to update subscriptions and thus used by hooks, and getState() which is used by imperative methods. I did this by moving the subscription portion back into its own hook.

I moved FieldHelpers to their own hooks as well, to enforce this boundary.

getFieldMeta -> useFieldMeta
getFieldHelpers -> useFieldHelpers
getFieldProps -> useFieldProps

Also fixed various other items and pushed back on items which can be addressed outside of this PR.

@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 17, 2021
@johnrom johnrom removed the stale label May 17, 2021
@johnrom
Copy link
Collaborator Author

johnrom commented Jun 7, 2021

closed in favor of #3231

@johnrom johnrom closed this Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants