sri icon indicating copy to clipboard operation
sri copied to clipboard

remove sComponentX overrides from RectComponent

Open chandu0101 opened this issue 8 years ago • 4 comments

Currently in ReactComponent we have override methods for componentWillUpdate,shouldComponentUpdate componentDidUpdate, componentWillReceiveProps just to get scala props/state from wrapper JSProps/JSState

 class ReactComponent extends .. {
   @JSName("sComponentWillUpdate")
  def componentWillUpdate(nextProps: P, nextState: S): Unit = ()

  @JSName("componentWillUpdate")
  override def jsComponentWillUpdate(nextProps: Props,
                                     nextState: State): Unit = {
    componentWillUpdate(nextProps.sprops, nextState.sstate)
  }

  @JSName("sShouldComponentUpdate")
  def shouldComponentUpdate(nextProps: P, nextState: S): Boolean = true

  @JSName("shouldComponentUpdate")
  override def jsShouldComponentUpdate(nextProps: Props,
                                       nextState: State): Boolean = {
    shouldComponentUpdate(nextProps.sprops, nextState.sstate)
  }

  @JSName("sComponentDidUpdate")
  def componentDidUpdate(prevProps: P, prevState: S): Unit = ()

  @JSName("componentDidUpdate")
  override def jsComponentDidUpdate(prevProps: Props, prevState: State): Unit = {
    componentDidUpdate(prevProps.sprops, prevState.sstate)
  }

  @JSName("sComponentWillReceiveProps")
  def componentWillReceiveProps(nextProps: P): Unit = ()

  @JSName("componentWillReceiveProps")
  override def jsComponentWillReceiveProps(nextProps: Props): Unit = {
    componentWillReceiveProps(nextProps.sprops)
  }

}

When ever a component updated via setState they will be called. even though implementation is just unit () we still paying method call! and user component should annotate method with @JSName("sShouldComponentUpdate"), etc how about removing this overrides ?

Current User Component with overrides of sComponetX


class HeaderComponent extends ReactComponent {

    @JSName("sShouldComponentUpdate")
  override def shouldComponentUpdate(nextProps: P,
                                       nextState: S): Boolean = {
   (nexProps ne props) && (nextState ne state)
  }

}

New after removing overrides from ReactComponent


class HeaderComponent extends ReactComponent {

  override def shouldComponentUpdate(nextJSProps: JSProps[P],
                                       nextJSState: JSState[S]): Boolean = {
 (nextJSProps.sprops ne props) && (nextJSState.sstate ne state)
  }

this way we can eliminate extra method calls , and no JSName annotations required, but user should call .sprops/.sstate when they need scala props/state.

chandu0101 avatar Feb 22 '17 13:02 chandu0101

How about just swap the names around, so instead of e.g.

@JSName("sShouldComponentUpdate") def shouldComponentUpdate

and

@JSName("shouldComponentUpdate") def jsShouldComponentUpdate

it's

@JSName("sShouldComponentUpdate") def sShouldComponentUpdate

and

@JSName("shouldComponentUpdate") def shouldComponentUpdate

and then just get rid of the JSNames. Maybe pick something nicer than sShouldComponentUpdate though.

nafg avatar Feb 22 '17 21:02 nafg

@JSName("sShouldComponentUpdate") def sShouldComponentUpdate

hmm , why we need @JSName same as method name :s

chandu0101 avatar Feb 22 '17 22:02 chandu0101

That was exactly my point. :)

On Wed, Feb 22, 2017 at 5:06 PM Chandra Sekhar Kode < [email protected]> wrote:

@JSName https://github.com/JSName("sShouldComponentUpdate") def sShouldComponentUpdate

hmm , why we need @JSName same as method name :s

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/chandu0101/sri/issues/62#issuecomment-281820091, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGAUAJWKaF5LchXut4bXx5sms6gOjLaks5rfLF2gaJpZM4MIohg .

nafg avatar Feb 22 '17 22:02 nafg

oh you want users override def sShouldComponentUpdate/newName ? , why not just remove this all together as it adds method extra method calls when component updated ...

chandu0101 avatar Feb 22 '17 22:02 chandu0101