Passing react element as label causes "Converting circular structure to JSON" error.

[tappleby]

This is similar to #33, I am trying to use react-intl components for translating the form labels, but the issue will happen for any react element:

<Input
  name="testInput"
  label={
    <FormattedMessage
      id="testInput.label"
      defaultMessage="Test label"
    />
  }
/>

This issue is caused by the JSON.stringify(label) in the component.getId mixin

I have a few ideas around fixing this, but not sure the best approach to take:

1. Only try and hash label if React.isValidElement returns false
2. Dont include the label when generating the id.
3. Require input label prop type to be string, use alternate method to get translation.

It would be nice if we could generate a unique id for each Formsy.Form component, then hashing the label wouldnt be necessary. Was thinking we could maybe opt into this using the parent context mixin.

[twisty]

Hi @tappleby -- thanks for your report and summary of approaches, that is helpful.

You may know already, but you can manually pass in an id prop and it will use that instead of generating an ID.

[twisty]

I may use something like this when label isn't hashable: https://gist.github.com/gordonbrander/2230317

[tappleby]

Yeah I'm using id in a few spots already to get around this. It's a pain to go back and add them to every input though.

The challenge with random IDs is making sure they only are generated once per component instead of every render.

[twisty]

Point taken about random IDs.

react-intl looks good, so I'd like this lib to "play nice" with it.

Thinking aloud, other solutions might be:

1. Render the label to a string using something like ReactDOMServer.renderToStaticMarkup and hashing that. Although I'm loathe to introduce a dependency like that just to solve this problem.
2. Generate an id in getInitialState or componentWillMount and use that as a fallback.

[tappleby]

Yea im not a fan of the renderToStaticMarkup approach.

Option 2 seems like it could work fine, is the hashed label approach used instead of something like this to handle server rendered markup?

react-intl does have another syntax which would also work but its kinda of annoying to convert every form element to:

<FormattedMessage id="testInput.label" defaultMessage="Test label">
  {label => <Input name="testInput" label={label} />}
</FormattedMessage>

[twisty]

Think this may be better solved with help from the mothership.

christianalfoni/formsy-react#367

[tappleby]

That looks like it would fix it. my only concern is how soon it would get merged.

[twisty]

Yes, good point. I guess you could go to the PR and 👍 it.

@Semigradsky do you have an opinion on christianalfoni/formsy-react#367 ? Is there a chance this will be merged?

[twisty]

@tappleby your concerns about time to merge were well founded. Pinging the awesome @aesopwolf to get an opinion on this PR 😸.

[aesopwolf]

Why not just delete this.hashString(JSON.stringify(label))?

[twisty]

Hello there @aesopwolf.

In plain old HTML, it's possible to have multiple controls in the same <form> sharing the same name attribute. Is this the same in formsy-react? That is: does formsy-react enforce 'uniqueness' of the name prop? (Is it possible to add two or more components to a formsy-react form with the same name prop?)

If not, we need something to add uniqueness for getting an ID for the component; I chose label (because it seems unlikely people will have two components with the same name and label).

[aesopwolf]

Ah you're right. I was under the impression that the name attributes had to be unique within a <form>.

formsy-react doesn't enforce uniqueness, but the last form element with a shared name will take priority and overwrite any previous values. (obviously not good).

I suppose you could use Math.random() in place of stringifying the label. Not ideal though. Your other PR looks fine to me. I'll pull the branch later tonight to give it a test. I have a few personal errands but I should be able to cut a release before end of day.

[tappleby]

Yea name wont be unique enough, especially if you have multiple form on page. Challenge with Math.random() is ids can change when props change. Worth checking to see how often getDefaultProps is called in the component lifecycle.

In my fork of FRC I ended up defaulting just to name but adding the option to provide a custom uid generator

We just use a static counter in our app:

let counter = 0;

function generateFieldUid(name) {
  return `frc-${name}-${counter++}`;
}

[aesopwolf]

I was thinking about it, since formsy-react doesn't support multiple inputs with the same name anyway then you shouldn't have to worry about this. The inputs are implicitly required to have unique names.

My vote is to still delete this.hashString(JSON.stringify(label)).

[tappleby]

Hrm, yea I guess formsy-react doesn't have the same limitations as the standard form API...

Since ids are global I could still see running into conflicts if 2 different forms with same name inputs are on the page. Could formsy-react support / generate a unique id for the form itself? Could use to prefix the input id.

[aesopwolf]

You're right... The issue that arises when 2+ different forms with the same name inputs are on the page is a matter of clicking the label and expecting it to focus the input nearby but instead it will focus the first input field on the page with a matching name.

The same behavior would happen in traditional html.

So that's more of a developer error than anything. I suppose library code could be written in formsy-react or even formsy-react-components to protect against this case. I'm not a fan though. I don't think the goal is to prevent people writing bad/invalid code.

[twisty]

Thanks for the feedback, I'm inclined to go along the route of just nuking the this.hashString(JSON.stringify(label)) and dealing with this in a future best practices / gotchas doc.

It's pretty easy to ensure unique name props when constructing your form (and I guess most people do this already).

If users really want to have to components with the same name, they'll then need provide a unique id prop (which is also possible now).