Learn advanced react and provide feedback on the workshop

[kentcdodds]

Hey folks! I'd love it if some of you could help me out with this workshop and make sure it's good to go! Please just follow the README instructions and let me know where you get stuck and what could be improved. Each of the exercises should be commented well enough for you to get everything done on your own without instruction.

Let me know if there are typos, legit errors, or anything that's unclear.

One thing I know is that the snapshots are failing on codesandbox. Hopefully I can get those working, but it should be working locally :) Thanks!

[FWeinb]

First of all this is extremely valuable! Thanks for all the effort. I worked throw it on codesandbox (Great platform for these courses).

Some of my findings:

02.js

// The button will be responsible for rendering the <Switch /> (with the right pros)

Not quite sure but I think this should be (with the right *props*)

03.js

// 💰: You'll need to move this below the `toggle` function. See
// if you can figure out why :)

Figured it out but would be nice to have a little explanation in the final version.

06.js

Even for the final version I get this error when clicking on the "on"/"off" button under the switch:

proxyConsole.js:72 Warning: This synthetic event is reused for performance reasons. If you're seeing this, you're accessing the property nativeEvent on a released/nullified synthetic event. This is set to null. If you must keep the original synthetic event around, use event.persist(). See https://fb.me/react-event-pooling for more information.

07.js

In line 68 you are using toggle as the argument name for the render prop. So to call the method toggle from getStateAndHelpers (line 45) you would have to write toggle.toggle. This is a deviation from 06.js where you are destructor the render prop arguments. I got a little bit confused by this because there are two things with the name toggle and wasn't sure how to add a reset method inside the toggle function of the react class. I hope this explanation makes any sense...

09.js

I would document the reasoning behind introducing stateChangeTypes (line 14) in the final solution and why it would make sense. I wrote these types inline as just e.g. {type: 'reset'} and called it a day but putting them there has some benefits. Makes it easer to understand the code in 10.js

10.js

There is another TODO: in line 25.
In line 30:

// We'll need a `getState` method here that returns a state object
// which has properties

I would add a 🐨to make it clear that the getState method should be implemented as described.
The description needs some more detail for my taste. I wasn't sure what to do here. Elaborating
on the distinction between controlled and uncontrolled props would make this clearer.

10-primer.js

I would switch the order between 10.js and 10-primer.js because this one has the explaination about controlled and uncontrolled props I needed in 10.js.

---

I find it very rewarding to work through these tiny problem sets. I can clearly see each piece of information fit right into each other. The amount of "work" in each step is perfectly measured and doesn't lead to any frustration.

Just one thing I don't really get. There are are some places where you are using 🐨as a marker for "work" and other times there is plain //TODO (e.g. in 03.js) maybe there is a distinction between these places but I would greatly appreciate a short description what each emojis stands.

It really shows that this is v2, astonishing how incredible the pacing of this is. Thanks for creating such great content!

Looking forward of doing the rest of it tonight.

[kentcdodds]

Thank you so much @FWeinb! This is very valuable feedback!

02.js

You're right, that's a typo. PR anyone?

03.js

Yeah, perhaps I should explain it in some comments 👍

06.js

That appears to be a bug. I'll need to look into it (or anyone else is welcome to address it in a PR if you so desire).

🐨 vs TODO

Anything that's TODO in the exercises should be 🐨. If it's not then I missed it 😅 PR for that too would be cool 👍

[FWeinb]

Glad you find this feedback valuable. I added my remaining thoughts on the other exercises. I could make a PR to fix these issues if you would like.

[kentcdodds]

Thanks a ton!

07.js

Yes, the naming is unfortunate. I totally understand what you're saying. I'll improve that.

09.js

Great point. I'll make that more clear 👍

10.js

Good points. I'll make that more clear too.

10-primer.js

That was a mistake. 10-primer.js is supposed to come before 10.js. Sorry 😅

Thanks again!

[AWolf81]

01 to 09.js

These excersises are good to do with your comments in the code.

10.js

For me it is still difficult to follow how the internalSetState method is working. I haven't used the mapping like you're doing it before. Maybe I have to read a bit more about it in the docs.

13.js

There is a "typo" in MyInput Component. Line 28 in excersise file not matching the code from the final file. So the input field is not updating properly. I can edit it and send a PR for this.

Also the commented import statement for Redux would be good at this example and also a brief description what the component should do would be great.

I like the idea of using Redux like in this example but I'm not sure if this is best practice. I think it's better to use one Redux store and use the Redux provider and connect the component to that store.
If that's right, you could add that as a comment there.

My thoughts to the workshop

You did a great job here. It was fun to work my way through it. Thanks for your work.
But I noticed that I'm still a React beginner at some parts - so especially at 10.js I had to peek at the solution to get the excersise done.

A possible improvement:
It would be good to have failing unit tests at each file because of these missing lines of the excersises. E.g. for 01.js a test like - it should toggle on state and it should call onToggle prop. So it would be easier to see if every part is properly implemented.

[kentcdodds]

Thank you so much for the help @AWolf81!

10.js

I think that I'll refactor from using the map calls to just creating new variable declarations instead. It's a little more verbose, but I think that it'll be a little less confusing for people. Thanks for that feedback.

13.js

Rendux isn't really intended to be a best practice or anything. People can implement it however they like. So I'm not entirely concerned about that bit so long as it works :)

A possible improvement:

Did you notice that there actually are unit tests for each exercise? As noted in the README you're supposed to swap the import of the final version with the import of the exercise and make the test pass. Was there something that I could have clarified there?

[dlannoye]

On exercise 8 I have a solution that passes the tests, but doesn't correctly call the state reducer with the correct arguments. My internalSetState code looks like this:

  internalSetState = (updates, callback) => {
    this.setState(this.props.stateReducer ? this.props.stateReducer(this.state, updates) : updates, callback);
  }

Which doesn't properly 'compute' the updates if a function is used before passing them to the stateReducer like you do in the final solution. If this isn't intended let me know and I can try to update the tests to handle this case.

[kentcdodds]

Yes, we'll want to add a test for this. I'm surprised it worked because the reset and toggle methods use both versions of the setState API 🤔

Oh, I know why it works. It just happens to work due to the simplicitic nature of the example. I'll add a test to make sure people don't make the mistake. Thanks!

[colinrobertbrooks]

In exercises-final/05.js, can't you delete toggle from getStateAndHelpers as it's not needed outside of togglerProps?

[kentcdodds]

Thanks @colinrcummings. As I noted in a PR that attempted to remove it:

This is still useful in some situations, so I definitely suggest continuing to expose this. Remember that prop getters are just helpers and should generally not be the only way to accomplish certain tasks :) Thanks anyway!

:)

[colinrobertbrooks]

@kentcdodds, how do you feel about the following to fix the displayName in exercises with HOCs?

withToggle (exercise 12):

code:

function withToggle(Component) {
  class Wrapper extends React.Component {
    render() {
      const { forwardedRef, ...rest } = this.props;

      return (
        <Toggle.Consumer>
          {toggle => <Component {...rest} toggle={toggle} ref={forwardedRef} />}
        </Toggle.Consumer>
      );
    }
  }

  Wrapper.displayName = `withToggle(${Component.displayName ||
    Component.name})`;

  const forwardRef = React.forwardRef((props, ref) => (
    <Wrapper {...props} forwardedRef={ref} />
  ));

  return hoistNonReactStatics(forwardRef, Component);
}

react devtools:

withRendux (exercise 13):

code:

function withRendux(Component) {
  class Wrapper extends React.Component {
    render() {
      const { forwardedRef, ...rest } = this.props;

      return (
        <Rendux.Consumer>
          {rendux => <Component {...rest} rendux={rendux} ref={forwardedRef} />}
        </Rendux.Consumer>
      );
    }
  }

  Wrapper.displayName = `withRendux(${Component.displayName ||
    Component.name ||
    'Wrapper'})`;

  const forwardRef = React.forwardRef((props, ref) => (
    <Wrapper {...props} forwardedRef={ref} />
  ));

  return hoistNonReactStatics(forwardRef, Component);
}

react devtools:

In exercise 13, I believe the displayName for StatePrinter isn't being passed through because it's a SFC in the same file (reference); here I added a a third possible displayName (Wrapper) to prevent it from showing withRendux(Unknown), which it does currently. In exercise 12, on the other hand, I believe the displayName for Subtitle is being passed through fine because it's a true Component.

[kentcdodds]

Ah yes, that makes sense. So a solution would be to change StatePrinter to:

const StatePrinter = withRendux(function StatePrinter({rendux}) {
  return (
    <div style={{textAlign: 'left'}}>
      state:
      <pre data-testid="printed-state">
        {JSON.stringify(rendux.state, null, 2)}
      </pre>
    </div>
  )
})

Anyone want to do that?