Minor cleanup work #555
Minor cleanup work #555
Conversation
| key={operation.name ? operation.name.value : '*'} | ||
| className={operation === highlight ? 'selected' : ''} | ||
| // tslint:disable-next-line | ||
| onMouseOver={() => onMouseOver(operation)} |
There was a problem hiding this comment.
Using React.SFC sounds very interesting!
What I'm a little bit concerned about, however, is the use of arrow functions, which is an antipattern for normal components, as the function reference of the arrow function changes on each rerender, so do the props for the element. What do you think about that?
There was a problem hiding this comment.
I was actually blocked by tslint for this very reason and wasn't sure what it was talking about. I think this particular instance is fine because a majority of the props which could initiate a rerender (in these two specific cases) are the ones that create the functions, which is why I added the tslint disable. I also learned my disable on tslint from another edited file which does the same thing for a simple lambda.
Long term my intention is to do another pass on these components and create a higher order component which passes a function directly into the SFC and only updates that specific function when components involved in it change. I was looking at recompose for this, but I'm open to suggestions.
For the short term, I'm not terribly concerned about the performance impact.
There was a problem hiding this comment.
Ok, thanks for giving your perspective on that one.
All other refactorings in this PR make a lot of sense to me, as they're simplifying a lot, but for this component, I don't see clear benefits for moving to this solution.
I'd rather stay with the normal component, which by the way could also be a pure react component.
We have severe performance problems in the Playground (which I'm looking into right now) - we have to be really mindful of changes like these.
So my suggestion: Putting the arrow functions back to instance methods and turning the component into a React.PureComponent
| key={operation.name ? operation.name.value : '*'} | ||
| className={operation === highlight ? 'selected' : ''} | ||
| // tslint:disable-next-line | ||
| onMouseOver={() => onMouseOver(operation)} |
There was a problem hiding this comment.
Ok, thanks for giving your perspective on that one.
All other refactorings in this PR make a lot of sense to me, as they're simplifying a lot, but for this component, I don't see clear benefits for moving to this solution.
I'd rather stay with the normal component, which by the way could also be a pure react component.
We have severe performance problems in the Playground (which I'm looking into right now) - we have to be really mindful of changes like these.
So my suggestion: Putting the arrow functions back to instance methods and turning the component into a React.PureComponent
|
@timsuchanek implemented changes. |
|
give me a sec. i'm rebasing |
- Stateless functional component
per discussion in #555
- Trying to move towards stateless, so I'm removing the pre-return render code.
|
All cleaned rebased and should have the changes we discussed. |
|
Thanks for the work @DavidJFelix ! |
|
No problem. Thanks for the collab. You'll probably see some more cleanup from me in the near future. |
* chore: remove empty file * chore: Convert ExecuteButtonOperation to a SFC - Stateless functional component * chore: make cleanup toggle and spinner * chore: make results more functional * chore: move execute button operation from SFC to purecomponent per discussion in graphql#555 * chore: remove pre-return commands from render in editor - Trying to move towards stateless, so I'm removing the pre-return render code.
I got a bit sidetracked with refactoring apollo-link. I was having a hard time groking 1000 line stateful components so I started doing some cleanup work wherever I saw low hanging fruit.
I'm working my way into the editor codebase a bit to make it a bit easier to swap out the fetcher.
Here are the granular commits that I have working so far.