Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Pass props and state through render #242

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Kampfkarren
Copy link
Contributor

Closes #199

Checklist before submitting:

  • Added entry to CHANGELOG.md
  • Added/updated relevant tests
  • Added/updated documentation

@coveralls
Copy link

coveralls commented Oct 9, 2019

Coverage Status

Coverage remained the same at 93.525% when pulling 1f2f162 on Kampfkarren:pass-props-react into deaa800 on Roblox:master.

Copy link
Contributor

@LPGhatguy LPGhatguy left a comment

Choose a reason for hiding this comment

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

Looks good to me!

I'll discuss the change with my team tomorrow and see if there are any reasons why we hadn't done this already. I know this is something that React hasn't done for whatever reason.

Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

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

This change seems pretty sound to me.

The only thing that's a little weird about this is that we still need to use self.props and self.state in other lifecycle methods (some of which already take args and it would be awkward to add these). Does that divergence bother us?

```lua
function MyComponent:render(props, state)
-- props == self.props
-- state == self.state
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to add a little bit more explanation of what we're trying to convey here. Maybe a little blurb that says that props and state can be accessed either as arguments or as properties on self?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that, I was just copying Preact's documentation.

@ZoteTheMighty
Copy link
Contributor

This still looks good to me, but I did some digging into why React doesn't do this, and here's what I found: reactjs/rfcs#4 (comment)

It's a pretty important point to make! Is it something we're worried about for Roact? Is it something we think we'll need to worry about? I may be missing something, but I think the closing-over-this problem they're talking about applies pretty equivalently to Lua as well as JS

@ZoteTheMighty
Copy link
Contributor

ZoteTheMighty commented Oct 15, 2019

One more thing to note as a followup: we do also have the same problem already with closing over initial props passed into init, so we should keep that in mind as well when we consider this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make StatefulComponent:render accept props and state as arguments
4 participants