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

Angular to React Migration #10271

Closed
2 tasks
stacey-gammon opened this issue Feb 9, 2017 · 19 comments
Closed
2 tasks

Angular to React Migration #10271

stacey-gammon opened this issue Feb 9, 2017 · 19 comments
Assignees
Labels

Comments

@stacey-gammon
Copy link
Contributor

stacey-gammon commented Feb 9, 2017

Angular to React Migration

What this is not

  • A complete rewrite

What this is

A slow migration to react, focusing on:

  • New apps being built from scratch
  • New features that are isolated
  • Leaf components

How to get started

When writing a new plugin from scratch

TODO
I have a plugin here that will serve as an example for how to write large react components in our code base when starting from scratch at the app level. Before this can serve as a guide:

  • Pick an end spot and finish the plugin, cleaning up all the code
  • Team wide code review of concepts and style

When writing a new large feature inside an already existing angular app

TODO
I will be working on rewriting some large parts of dashboard in react that can serve as an example for this specific situation. This will also go through a team wide code review prior to it being accepted as an example we should follow.

Migrating leaf components

  1. Identify stateless leaf components
  2. Create those leaf components (possibly in the ui framework depending on how generic they are).
  3. Work your way up the dom and migrate the next level into a component.

Goals

  • Middle to large sized chunks of react code inside angular
  • Prefer stateless components
  • Avoid tons of react roots scattered throughout the code

Examples

  • React Icons => Landing Page Table Header.
    1. Identify and migrate the smallest examples - React Icons
    2. Work up the DOM - React Buttons
    3. Find a place to stop that is large enough to justify the angular -> react glue. Haven't hit this yet, but the next iteration would likely be a landing page table header.
    4. Check in your work and either start over again or continue working up the dom (e.g. Table Header => Landing Page Table).

Gotchas

  • The angular -> react glue (ngreact) creates a new root for every "entry point". These components can't talk to each other through the react framework.
  • I don't think ngreact components handle children, at least they didn't when I was using react-component, but I didn't try when using reactDirective. The following will not fill props.children:
<react-component name="ButtonComponent">
 <a href="link">
   <react-component name="PlusIconComponent"></react-component>
 </a>
</react-component>
  • When using reactDirective, be sure to use MyComponent.propTypes. This tells the directive what attributes to look for and what props to pass.
  • Be careful when passing props that are also valid html attribute names. For example, passing a tooltip prop cause both the react component to display a tooltip, and the angular side of the component as well.

Style Guidance

  • Prefer Stateless functional components where possible. When it isn't possible, use ES6 style React Classes.
  • Pass functions via props to avoid storing state in a component.
  • Prefer reactDirective over react-component

Kibana Resources

Ground up plugin example:

  • React Examples - A proof of concept plugin, written from scratch. It's a WIP and not yet ready for a code review but will eventually be an example of how we want our react code to look. Features
    • Flux Architecture
    • Angular hooks at the top level.

External Resources

@uboness
Copy link

uboness commented Feb 12, 2017

Few comments:

I have a plugin here that will serve as an example

The example uses redux... I'm not aware of any decision to use redux.

Prefer Stateless functional components where possible. When it isn't possible, use ES6 style React Classes.

this does not bring consistency to the code base.... lets decide on one way and follow it across the board.

Pass functions via props to avoid storing state in a component.

I don't understand (sorry)... so functions are considered state?

@stacey-gammon
Copy link
Contributor Author

stacey-gammon commented Feb 12, 2017 via email

@kimjoar
Copy link
Contributor

kimjoar commented Feb 12, 2017

Prefer Stateless functional components where possible. When it isn't possible, use ES6 style React Classes.

this does not bring consistency to the code base.... lets decide on one way and follow it across the board.

If we plan to move forward with #8315, I think a good baseline is to follow the Airbnb React guidelines too, which has basically the same recommendation as @stacey-gammon (see https://github.com/airbnb/javascript/tree/master/react#class-vs-reactcreateclass-vs-stateless). In my experience this doesn't cause any consistency problems in the codebase, and is very clean and readable (we follow this in Cloud UI too).

@uboness
Copy link

uboness commented Feb 12, 2017

If we plan to move forward with #8315, I think a good baseline is to follow the Airbnb React guidelines too, which has basically the same recommendation as @stacey-gammon (see https://github.com/airbnb/javascript/tree/master/react#class-vs-reactcreateclass-vs-stateless). In my experience this doesn't cause any consistency problems in the codebase, and is very clean and readable (we follow this in Cloud UI too).

cool... I would still love to understand the rational behind this. I get it that functions are safer... but when state is involved we move to classes.... this makes little sense to me. I still don't understand why we can't have one way of doing something instead of having two.

@uboness
Copy link

uboness commented Feb 12, 2017

The plugin example doesn't use redux, it uses flux. As far as I am aware
of there was no decision on any specific state mechanism to use, so I just
picked something to start with.

I guess it was wrong of me to assume we're using redux based on the redux dependency in package.json (why do we have this dependency?)

We've agreed to move to React and I believe the examples that we offer should solely focus on how to write react components.

@kimjoar
Copy link
Contributor

kimjoar commented Feb 12, 2017

It's just cleaner for the normal case (aka when you don't have state in the component). It also signals clearly that the component doesn't rely on state or other lifecycle events. We use them a whole lot in the "leaf components", as those are usually pure "render your html with the props I'm giving you" style components.

@uboness
Copy link

uboness commented Feb 12, 2017

so basically, we rather no be consistent and have one way of doing something just so instead of:

class Label extends React.Component {
  render() {
    return <div>{this.props.text}</div>;
  }
}

we do:

function Label(text) {
  return <div>{text}</div>;
}

?

(as a side note... the former clearly indicates to me that it's a react component, the later doesn't)

@kimjoar
Copy link
Contributor

kimjoar commented Feb 12, 2017

Yep. I don't see any problems in that. It's super-easy to add an eslint rule for it too, then we're consistent across the code base. Then we're also consistent with the "recommended approach" (whereby "recommended" == the styleguide we've said we want to rely on)

@stacey-gammon
Copy link
Contributor Author

stacey-gammon commented Feb 12, 2017 via email

@uboness
Copy link

uboness commented Feb 12, 2017

@stacey-gammon thx all the background...

I think it's important that we indeed scope this appropriately.

@uboness
Copy link

uboness commented Feb 12, 2017

Yep. I don't see any problems in that. It's super-easy to add an eslint rule for it too, then we're consistent across the code base. Then we're also consistent with the "recommended approach" (whereby "recommended" == the styleguide we've said we want to rely on)

interesting....

@cjcenizal
Copy link
Contributor

FYI, the Redux dependency is there because the UI Framework documentation site is built using Redux.

@epixa
Copy link
Contributor

epixa commented Feb 13, 2017

In addition to what's been said, stateless components have two important benefits:

  1. The components themselves aren't React, they're just pure javascript functions. This means you don't need to have any knowledge of React component lifecycles to write them, you don't need to do any sort of mocking or manual control of component lifecycles when testing them, and we have one fewer coupling point to a dependency for the majority of components in the system.
  2. We can get performance gains for free for the majority of our components. Since there is no possibility of tying into lifecycle events and such, React core can optimize for performance around stateless components.

@uboness
Copy link

uboness commented Feb 13, 2017

The components themselves aren't React, they're just pure javascript functions. This means you don't need to have any knowledge of React component lifecycles to write them, you don't need to do any sort of mocking or manual control of component lifecycles when testing them, and we have one fewer coupling point to a dependency for the majority of components in the system.

you have tight coupling to React anyway regardless of how you look at it. I believe that this argument of trying to minimize the dependency on react comes from the concern of "what happens if one day we want to move away from React... using the function notation will reduce that work". This is a false assumption, as migration will probably require to redefine all components class & functions alike + their unit tests (the fact that React accepts a the function notation is a feature of React). For the rest, I assume the test infra will be there to help easily write simple tests for React components, in which case we should not foresee an overhead in writing these for the simplest components as well.

We can get performance gains for free for the majority of our components. Since there is no possibility of tying into lifecycle events and such, React core can optimize for performance around stateless components.

IMO, first/only valid reason so far... that said, not sure whether or not to categorize it as pre-mature optimization. Do we know, by experience/facts, that this has a significant/meaningful impact?

@kimjoar
Copy link
Contributor

kimjoar commented Feb 13, 2017

It doesn't, it's currently the same performance.

Long-term the React team plans to move away from classes (https://twitter.com/dan_abramov/status/817391956041101316, https://twitter.com/dan_abramov/status/745757519994294281). Still early stages, though, so classes will be there for a while.

In my experience it improves readability quite a bit (for a couple of reasons: no this.props just destructured props, it usually pushes towards smaller components that are easier to test, it makes it easier to extract small "component helpers" using just arrow functions, etc). Probably counts in your "not valid" bucket, but in my experience writing React full-time for years it's been a nice plus to the code.

@uboness
Copy link

uboness commented Feb 13, 2017

@kjbekkelund actually having React move away from classes is a perfectly valid reason IMO... I was not aware that was the case (and I'm quite surprised as in the past they were promoting classes). Thx for the reference.

it makes it easier to extract small "component helpers" using just arrow functions, etc). Probably counts in your "not valid" bucket, but in my experience writing React full-time for years it's been a nice plus to the code.

can't say if I this is valid for me or not... simply because I don't understand what it means and how it looks like vs. the alternative.

@stacey-gammon
Copy link
Contributor Author

As per discussion with @uboness and @epixa - Split out the Leaf Components section into it's own issue so we can focus on that aspect separately - #10312. Once that issue is closed we can move on to the larger efforts.

@cjcenizal
Copy link
Contributor

This process of migrating from Angular to React will be supported by our process of rebuilding our UI with the EUI Framework (#15282).

@cjcenizal
Copy link
Contributor

EUIfication will carry the torch for this process, closing.

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

No branches or pull requests

6 participants