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

Form persistence #561

Open
wants to merge 44 commits into
base: main
Choose a base branch
from
Open

Form persistence #561

wants to merge 44 commits into from

Conversation

aadito123
Copy link
Member

Attempt at addresssing #503

This PR includes, the form-persist-core package with tests and the changes to the form-core which make it work. The form-persist-core exports two main things: class PersisterAPI and the helper function createFormPersister.

The persister is an optional field to the FormApi class, which when passed, triggers restoring and persistence on FormState changes. For persisting to localStorage, it is:

const persisterAPI = new PersisterAPI({ storage: window.localStorage })
new FormApi({ persister: createFormPersister(persisterAPI, 'myFormAPI') })
new FormApi({ persister: createFormPersister(persisterAPI, 'myFormAPI2') })

The storage field can also be async, as inspired by the persister in TanStack/query.

Opinions needed for:

  • API for framework adapters. For React and Solid adapters, I am thinking there should be a global provider for the PersisterAPI, and on then something like useForm({ persistFormKey: 'myFormKey' }) or useForm({ formPersister: useFormPersister('myFormKey') }) would be all that is needed. This way the dev can also opt out of persistence for a specific form. Very open to opinions and suggestions. Don't have anything in mind for the Vue version but I don't see why it cant just be the same thing.

  • Persistence state i.e. isRestored and isRestoring are part of the FormState, but I don't really like having that there. Would it be better to have it in the PersisterAPI itself? If it is moved into the persister, we will need some way to pass that state back to the user, which might necessitate something like 'useIsRestoring` or something similar. Again, open to opinions and suggestions.

  • Fine-grained persistence. This might be more of a future thing, but perhaps, there should be a way to opt-out of having certain fields in the persistence. This could certainly just be done by ignoring the form-persistence-core package and passing in a completely custom Persister to the FormApi that can process the FormState as wanted. Just a thought.

  • Custom serializer. Right now I am just using JSON.parse and JSON.stringify to serial the form state. It should be more than enough considering that state is only strings and numbers. TanStack/query allows their persister to have custom serializers. I don't see the usecases for forms though, although open to hear them, if any. It is very easy to add it in.

To be done:

  • DOCS! Haven't been pulling my weight with the docs so will try to include some in follow up commits.
  • Framework adapters. I feel confident in making the React and Solid adapters but might need help with the Vue one.

Am I missing anything else here?

Copy link

codesandbox-ci bot commented Jan 8, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@crutchcorn
Copy link
Member

This is incredible work! I'll have to think more about this issue when I have a bit more time, but at first glance(s) I think you're more than on the right track.

I'll follow-up this weekend sometime to add thoughts on what we have opinions on. In the meantime, let's see if the community has any thoughts on this API

@crutchcorn
Copy link
Member

Buckle in, folks, this is a long one!

Framework Adapter API

  1. On the note of Framework adapters, I'd like us to do a modification of the second syntax you suggested:
useForm({
    formPersister: useFormPersister({
        persistKey: ['myFormKey']
    })
})

This would do a few things for us:

1a) Match TanStack Query's APIs for a more consistent cross-ecosystem feel and consistency in codebases (able to follow @TkDodo's caching key suggestions by keeping related keys in an object/functions)
1b) By using a hook here, we might be able to add additional memoization like useMemo or whatnot if we need to rely on that internally. It overall gives us as maintainers more flexibility

1c) useFormPersister allows us to return a memoized FormPersisterAPI.ts instance with an update function ran during renders, which aligns with how all of our other hooks work (more or less), so there's consistency in implementation

1d) We'd then be able to pass additional keys to modify the persistence functionality. IE:

useForm({
    formPersister: useFormPersister({
        persistKey: ['myFormKey'],
        omitKeys: ['password']
    })
})

Persisted State APIs

  1. I'd like to propose one of two APIs to solve this:

2a) Mutate form using useFormPersister:

const form = useForm({
    defaultValues: {/*...*/}
})

const {isRestoring, isRestored} = useFormPersister({
    form,
    persistKey: ['myFormKey'],
    omitKeys: ['password']
})

Which would allow us to reuse the mergeForm function (currently only used for SSR) to do something like:

const useFormPersister = ({form, persistKey, omitKeys}) => {
	const {state, persister} = getFormPersister({persistKey, omitKeys})
	form.persister = persister;
    // Mutates `form` state, doesn't return a new instance
    mergeForm(form, state)
}

2b) Use the API outlined in #1 to extract from the formPersister API directly:

const form = useForm({
    formPersister: useFormPersister({
        persistKey: ['myFormKey'],
        omitKeys: ['password']
    })
})

// Under-the-hood, these fields are part of the FormPersisterAPI.ts file, not FormAPI.ts
const {isRestoring, isRestored} = form.formPersister;

Fine-Grained Persistence

  1. I think we solved this using the API outlined in Documentation inconsistent #1, which enables us to exclude or include keys either like:

3a)

useForm({
    formPersister: useFormPersister({
        persistKey: ['myFormKey'],
        omitKeys: ['password'], // Or maybe: (fieldName, fieldValue) => key !== password (?)
        // Or
		includeKeys: ['name']
    })
})

3b)

useForm({
    formPersister: useFormPersister({
        persistKey: ['myFormKey'],
        shouldPersist(fieldName, fieldValue) {
			if (fieldName === "password") return false;
            return true;
		}
    })
})

Custom Serializer

  1. I vote that we allow support for custom serializers. This does a few things for us as well:

4a) Consistency with TanStack Query

4b) Persisting built-in objects like Dates using JSON5 without us having to roll it as a dependency

Copy link

nx-cloud bot commented Mar 11, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 0c24ff2. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@aadito123
Copy link
Member Author

I think just about everything is implemented. Next great effort will come in writing the docs and examples. Would love help on that.

Notes:

  1. Framework Adapters
    Think we are basically on the same page. For the formKey, I left the type as a string, since I am not sure what the advantage of having an array would be. My understanding is that for Query, the array is used for cache invalidation at different levels. I am not sure if this would be applicable to the form persister. As such, I made the API similar to the Query persister.

  2. Persistence APIs
    I found 2b to be the cleanest, but made a few modifications. I attached the restore states to the store on the FormApi instance itself. This gives us the watch and Subscribe functionality for free. My main concern is that it pushes quite a bit of persistence logic into the form-core but I am not sure how else we could make it work, since the restore function needs to be called in the onUpdate for the store. Definitely would like some more suggestions for this.

3 & 4. Totally agreed, so went ahead and added those in, too.

@aadito123 aadito123 requested a review from crutchcorn March 11, 2024 15:35
@aadito123 aadito123 marked this pull request as ready for review March 20, 2024 14:43
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 80.19802% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 87.69%. Comparing base (51ef17f) to head (0c24ff2).

Files Patch % Lines
packages/form-core/src/FormApi.ts 64.70% 11 Missing and 1 partial ⚠️
packages/form-persist-core/src/index.ts 79.48% 7 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #561      +/-   ##
==========================================
- Coverage   88.91%   87.69%   -1.23%     
==========================================
  Files          28       33       +5     
  Lines         812      894      +82     
  Branches      187      205      +18     
==========================================
+ Hits          722      784      +62     
- Misses         83      101      +18     
- Partials        7        9       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

3 participants