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

fix(conform-react): stops managing all inputs by default #775

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

Conversation

edmundhung
Copy link
Owner

@edmundhung edmundhung commented Sep 17, 2024

Fix #729

Related Issues:

This stops Conform from managing all inputs, except those that are configured with getInputProps, getSelectProps and getTextareaProps. You can customize the behavior by setting shouldManagerInput on the useForm hook, e.g.

function Example() {
  const [form, fields] = useForm({
    shouldManageInput(element) {
       // To manage all inputs except the csrf-token input
       return element.name !== 'csrf-token';
    }
  });

  // ...
}

If we decide to go with this solution, we can also deprecate shouldDirtyConsider later as it could be derived based on the result of shouldManageInput.

Todos:

  • Regression tests
  • Docs update

Copy link

changeset-bot bot commented Sep 17, 2024

⚠️ No Changeset found

Latest commit: b95eb3d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

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

Click here to learn what changesets are, and how to add one.

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

Copy link

pkg-pr-new bot commented Sep 17, 2024

Open in Stackblitz

More templates

@conform-to/dom

pnpm add https://pkg.pr.new/@conform-to/dom@775

@conform-to/validitystate

pnpm add https://pkg.pr.new/@conform-to/validitystate@775

@conform-to/react

pnpm add https://pkg.pr.new/@conform-to/react@775

@conform-to/yup

pnpm add https://pkg.pr.new/@conform-to/yup@775

@conform-to/zod

pnpm add https://pkg.pr.new/@conform-to/zod@775

commit: b95eb3d

@edmundhung edmundhung changed the title fix(conform-react): avoid managing inputs that are not configured via getInputProps fix(conform-react): avoid managing inputs that are not configured with getInputProps Sep 17, 2024
@edmundhung edmundhung changed the title fix(conform-react): avoid managing inputs that are not configured with getInputProps fix(conform-react): stops managing all inputs by default Sep 17, 2024
Copy link

cloudflare-workers-and-pages bot commented Sep 17, 2024

Deploying conform with  Cloudflare Pages  Cloudflare Pages

Latest commit: b95eb3d
Status: ✅  Deploy successful!
Preview URL: https://9490c437.conform.pages.dev
Branch Preview URL: https://opt-in-auto-value-update.conform.pages.dev

View logs

Comment on lines +69 to +71
shouldManageInput?: (
element: HTMLInputElement | HTMLSelectElement | HTMLTextAreaElement,
) => boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

don't forget buttons! :)

Copy link
Owner Author

@edmundhung edmundhung Sep 18, 2024

Choose a reason for hiding this comment

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

Conform does not manage any buttons (i.e. It should not update the button value). Is there something you want to achieve with this option?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, okay, I was thinking user intent buttons (not conform intent buttons) but if thats outside this scope, please ignore my comment.

@lukasa1993
Copy link

would this have any way to go back to 1.1.5 flow ? currently we are using many hidden fields with bunch of names listing them one by one will be extremely tedious

@edmundhung
Copy link
Owner Author

edmundhung commented Sep 18, 2024

would this have any way to go back to 1.1.5 flow ? currently we are using many hidden fields with bunch of names listing them one by one will be extremely tedious

Unless you are configuring the hidden inputs with helpers like getInputProps, this PR should make it work like 1.1.5. Conform will only updates inputs configure using those helpers.

You will have access to the dom element, so feel free to do any logic that makes sense to you, e.g.

function Example() {
  const [form, fields] = useForm({
    shouldManageInput(element) {
       // Ignore all inputs with `address.` as the prefix
       return !element.name.startsWith('address.');
       
       // Ignore all hidden inputs
       return element.type !== 'hidden';

       // Otherwise, turn it off completely
       return false;
    }
  });

  // ...
}

@lifeiscontent
Copy link
Contributor

@edmundhung this works for me! 👍🏻

@edmundhung
Copy link
Owner Author

edmundhung commented Sep 19, 2024

Some people might still consider this a breaking change if they use the helper functions to configure inputs. One alternative could be to make this entirely opt-in for v1 and switch to opt-out in v2. However, that would mean the helpers would still return the key property and trigger the React warning. To avoid confusion, I might just deprecate the helpers now, as they don't integrate well with component libraries. I'd rather clearly define the properties they should pass to inputs and encourage them to create their own helpers to fit their needs.

@lifeiscontent
Copy link
Contributor

@edmundhung I'd be open to that, however there should be some dedicated types exposed for things like the return value of getFieldset() and getFieldList() I personally have had to reach into @conform-to/dom to access the Combine type for things, it would be nice not to have to do that since I'm only using @conform-to/react

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