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

Components are double-rendering elements #14

Closed
spiffytech opened this issue Jan 21, 2021 · 10 comments
Closed

Components are double-rendering elements #14

spiffytech opened this issue Jan 21, 2021 · 10 comments

Comments

@spiffytech
Copy link
Member

I'm creating a form that adds/removes fields based on a radio selection. The initial render is fine, but changing the radio value causes the radio buttons/labels to double-render and one of the form fields to double-render. I can't spot any mistakes in my component code that would explain this behavior.

Here's a sandbox demonstrating the issue.

@jeswin
Copy link
Member

jeswin commented Jan 21, 2021

I'll look at it right away.

@jeswin
Copy link
Member

jeswin commented Jan 21, 2021

@spiffytech You sandbox was very helpful.

Fixed in 0.0.31. There were a couple of issues. One related to how we handle nulls, and another silly one related to handling child nodes. Sorry about these bugs, I don't have tests to cover all of these cases yet (working on it).

You might actually find a few more. I'll try to fix them ASAP.

You could close this if it works for you.

@spiffytech
Copy link
Member Author

I can confirm that with v0.0.31 the sandbox works correctly. In my project, the radios render correctly now, too.

However, I can still trigger a double render of the password element. My first sandbox simplified things a little to help with minimal reproduction, but it appears my complete project code still triggers the bug.

Here's a copy/paste out of my project, everything but the data handling logic. I added a render counter, and I can see that the password component rendered once and never updated. On subsequent rerenders, a duplicate component is inserted/removed correctly.

@jeswin
Copy link
Member

jeswin commented Jan 21, 2021

Let me take a look at it.

@jeswin
Copy link
Member

jeswin commented Jan 21, 2021

Ok, so you're dealing with a small quirk forgo has.

The component ctor (function PrettyLabel) and the render() method it returns are both called with the (props, args) the first time around. Subsequently, only the render() gets called.

Your original code did not take parameters in the render() method. Fixed code below.

function PrettyLabel({
  label,
  children,
}: forgo.ForgoElementProps & PrettyLabelProps): forgo.ForgoComponent<
  forgo.ForgoElementProps & PrettyLabelProps
> {
  return {
    render({ label, children }) {
      return (
        <label className="text-gray-700 block">
          <span>{label}</span>
          {children}
        </label>
      );
    },
  };
}

@jeswin
Copy link
Member

jeswin commented Jan 21, 2021

I think this aspect definitely needs to be made clearer in the README.

@spiffytech
Copy link
Member Author

Yep, that change works in my project.

Can you help me understand better what's going on under the hood here? My mental model of Forgo predicted that if I didn't reaccept props in render(), each rerender would just replace the component's elements with new elements containing outdated values. I can't piece together from my current knowledge why the render algorithm would instead output two DOM nodes where there should be one.

@jeswin
Copy link
Member

jeswin commented Jan 21, 2021

Sure.

Actually it wasn't outputing two DOM nodes, just not discarding the old value.

So you have Log in - with 2 fields

  • Username
  • Password

Sign Up with 4 fields

  • Username
  • Email
  • Password
  • Repeat Password

When you switch from Log in to Sign Up

  • DOM currently contains 2 nodes (Username and Password)
  • Forgo now has 4 nodes to render
  • It skips the first two because PrettyLabel is rerendering with old values (args from Component Ctor, ie the function PrettyLabel)
  • It renders two additional PrettyLabel components because they were missing from the DOM.

So you have:

  • Username from Login
  • Password from Login
  • Password from Sign up
  • Repeat Password from Sign Up

This is what I think was happening.

@jeswin
Copy link
Member

jeswin commented Jan 21, 2021

I have updated the README to make this distinction clearer - but I could probably do better here.

One option is to have a debug mode, in which we detect the incorrect usage of initialProps automatically and alert the developer.

@spiffytech
Copy link
Member Author

Ah, that makes complete sense. Somewhere along the line I got the assumption that args.element would be a stable value, but it makes total sense why it wouldn't be. Thanks!

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

No branches or pull requests

2 participants