Skip to content
This repository has been archived by the owner on Jul 28, 2019. It is now read-only.

Re-implement reactDirective to be more angular-oriented #11

Closed
wants to merge 6 commits into from

Conversation

thenikso
Copy link

With reference to issue #10, this PR has the following goals:

  • Re-implement reactDirective to accept a React component instead of an injectable name as its first parameter
  • Re-implement reactComponent to use reactDirective

This PR contains breaking changes to the current usage of reactDirective. It also requires AngularJS 1.3+

TODO:

  • reactDirective should accept a React component as its first parameter
  • reactDirective should sanity check its parameters
  • reactDirective should update the view when changes in the angular models occurs
  • reactComponent should be implemented using reactDirective
  • React components rendered via reactDirective should have the means to update angular models when updating React props

…t component instead of the name of an injectable one.

This is a BREAKING change as `reactDirective` will not work if a string is passed as a first parameters instead of a React class.
The directive generated with `reactDirective` no longer uses `replace` and does now isolate it's scope.
AngularJS 1.3+ is now required as the implementation uses `$watchGroup` to watch bind properties. Also, for performance reasons, properties are no longer watched for object equality.
@hasdavidc
Copy link
Collaborator

@thenikso - thanks for the PR, solid stuff. We'll look through it and update with thoughts. From a quick glance, we may want put this in its own separate feature branch until all those TODOs get done

@thenikso
Copy link
Author

I'm not sure that the way React is meant to be used when updating it's state maps nicely to Angular.
It's still not clear to me weather the scope of a ngReact directive should map to props or state of a React component.

Anyway, if a React component, for example, renders a text input whose initial value is backed by an Angular scoped value; it should be possible to retrieve the changed state.

A React component handles its initial values via externally provided props and internally updates its state. That state is what ultimately drives the component rendered data, so that should be exposed by ngReact somehow. Ideas are welcome!

@kasperp
Copy link
Collaborator

kasperp commented Oct 25, 2014

Thanks for your pull request. Here are my thoughts on some of your points.

I'm not sure that the way React is meant to be used when updating it's state maps nicely to Angular.
It's still not clear to me weather the scope of a ngReact directive should map to props or state of a React component.

state is mean to be internal to the React Component, so for I think that props is the right mechanism to get data into the React Component.

Anyway, if a React component, for example, renders a text input whose initial value is backed by an Angular scoped value; it should be possible to retrieve the changed state.

I'm not sure that I agree with this (or I don't fully understand what you mean). Props are immutable and shouldn't be changed within the React component. State is internal the component and shouldn't be accessed from the outside. So any "state change" should be done with callbacks provided as props to the react components. This means that updates can be done in the angular controller.

Here is an example of the approach I have taken to deal with updates:

var app = angular.module( 'app', ['react'] );

app.controller( 'mainCtrl', function( $scope ) {
  $scope.person = { fname: 'Clark', lname: 'Kent', age: 28 };

  $scope.birthday = function(){
    $scope.person.age++;
  };
} );

app.value( "Hello", React.createClass( {
  propTypes: {
    fname: React.PropTypes.string.isRequired,
    lname: React.PropTypes.string.isRequired,
    age: React.PropTypes.number.isRequired,
    birthday: React.PropTypes.func.isRequired
  },

  handleClick: function() {
    this.props.birthday();
  },

  render: function() {
    return React.DOM.span( { onClick: this.handleClick },
      'Hello ' + this.props.fname + ' ' + this.props.lname + ' ' + this.props.age
    );
  }
} ) );

app.directive( 'hello', function( reactDirective ) {
  return reactDirective( 'Hello' );
} );
<p ng-controller="mainCtrl">
  <hello fname="person.fname" lname="person.lname" age="person.age" birthday="birthday"/>
</p>

Another approach could be to further explore these ideas
https://developers.mobileapptracking.com/addressing-angular-weaknesses-with-react-and-flux/

@kasperp
Copy link
Collaborator

kasperp commented Oct 26, 2014

@thenikso Thanks again for you pull request, I have taken a different approach to allow react components to be passed into reactDirective as suggested in #10. See branch react-directiv-accepts-react-components. There are no breaking changes with the approach i have taken.

@thenikso
Copy link
Author

I like your approach @kasperp, it clearly separate Angular and React.

At this point I think it could be considered using attrs.$observe instead of a watch given that attributes does not need to have a double binding.

@kasperp
Copy link
Collaborator

kasperp commented Oct 27, 2014

At this point I think it could be considered using attrs.$observe instead of a watch given that attributes does not need to have a double binding.

Nice - I'll give that a go.

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