Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

Make branch return a stateless functional component #273

Merged
merged 1 commit into from Nov 8, 2016
Merged

Make branch return a stateless functional component #273

merged 1 commit into from Nov 8, 2016

Conversation

matthieuprat
Copy link
Contributor

@matthieuprat matthieuprat commented Nov 7, 2016

Not sure you would want to merge this but at least, I will learn something out of the discussion!

Currently, branch returns a full-fledged React component. I am wondering whether it could return a stateless functional component instead (which is interesting because the component could be eagerly rendered).

That would essentially mean that we would be fine with factories being (lazily) initialized in render instead of the component's constructor and the willComponentReceiveProps hook—is it the case?

@istarkov
Copy link
Contributor

istarkov commented Nov 7, 2016

For now branch could be used as

branch(
   testFn,
   compose(
      Hoc0,
      ...,
      HocN,
   ),
   compose(
      AnotherHoc0,
      ...,
      AnotherHocN,
   ),
)

so in it's base it allows to branch enhancers, no need to return React.Component

@matthieuprat
Copy link
Contributor Author

Very true.

I'm not talking about branch's parameters though, but its return value.

Currently, the branch HoC returns a class component—which is never referentially transparent and which cannot be eagerly rendered as a result.

My proposed changed is about modifying branch's implementation to return a stateless, referentially transparent, functional component. This way, it may be eagerly rendered.

@matthieuprat
Copy link
Contributor Author

matthieuprat commented Nov 7, 2016

I bet we could do the same for withHandlers by the way. If this PR is accepted, I will be happy to submit another one for withHandlers.

Edit: I might have been a little too optimistic :)
After giving it a second thought, I can't figure out how we could make withHandlers return a SFC and keep handlers immutability at the same time.

@istarkov
Copy link
Contributor

istarkov commented Nov 7, 2016

Got it, like it, let us think a little

@istarkov
Copy link
Contributor

istarkov commented Nov 7, 2016

I see no issues with this PR, I like it more than current version @wuct @acdlite ?

@wuct
Copy link
Contributor

wuct commented Nov 8, 2016

I don't see any issue either. I like functional stateless components :)

@istarkov istarkov merged commit 2b72c17 into acdlite:master Nov 8, 2016
@istarkov
Copy link
Contributor

istarkov commented Nov 8, 2016

Thank you!!! So nice to see such changes!

@matthieuprat matthieuprat deleted the branch-sfc branch November 8, 2016 09:41
@matthieuprat
Copy link
Contributor Author

Thank you for your responsiveness!

Side note: the new implementation will only ever call the left and right arguments at most once (whereas the former implementation called them (at most) once per instance).

I see that as a feature rather than a bug but it's worth noting that if the left and right arguments are not pure functions, branch may behave strangely.

Here is a test to illustrate that:

test('left argument is called at most once', t => {
  let i = 0
  const impureHoC = _ => {
    i++
    return _ => <div>{i}</div>
  }

  const BranchedComponent = branch(
    _ => true,
    impureHoC
  )(_ => null)

  t.is(0, i)

  mount(<BranchedComponent />)
  t.is(1, i)

  mount(<BranchedComponent />)
  t.is(1, i)
})

The new implementation would pass this test. The former would fail on the last assertion.

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

Successfully merging this pull request may close these issues.

None yet

3 participants