tests and fixes #457 #472
tests and fixes #457 #472
Conversation
|
Why did you close the other PR? |
|
|
||
| notify() { | ||
| current = next | ||
| for (let i = 0; i < count; i++) { |
There was a problem hiding this comment.
Please remove this count variable. It risks things getting out of sync somehow. Calling .length is just as fast, especially in engines like v8 that use hidden classes.
|
github did it automatically when i deleted my branch and recreated it. I wanted to have the failing test before the fix so I was diddling with my git history. Ended up messing up my branches and just squashed the changes and recreated the branch. |
|
You can just fix that locally and force push next time. A branch is just a pointer to a particular commit hash, and force pushing lets you change the remote branch/pointer to a commit not along the same chain. Github fixed handling that case a year or two ago, so it no longer loses all your comments and stuff like before. |
| {this.getChildComponent(this.state.location.pathname)} | ||
| </App>) | ||
| } | ||
| } |
There was a problem hiding this comment.
From one of the react-router maintainers, this is a top quality mock router 😄
There was a problem hiding this comment.
Agreed. Easy to extend as well for the next person who wants to hoist it and have it take a map of route to component.
| @@ -1,45 +1,61 @@ | |||
| // encapsulates the subscription logic for connecting a component to the redux store, as | |||
| // well as nesting subscriptions of descendant components, so that we can ensure the | |||
| // ancestor components re-render before descendants | |||
There was a problem hiding this comment.
I know this is a bit out of scope, but perhaps we can mention the whole current/next setup below and why it's structured that way, now that all the code is in one spot right below this. It's not super-obvious why you would do that just reading through (do we have tests for Subscription? That would be another way of documenting it.)
There was a problem hiding this comment.
The next/current pattern is straight out of the redux source. My hope is to eventually refactor that code into something reusable by the main store or Subscription.js
There was a problem hiding this comment.
I know @acdlite wanted to do something like that in reduxjs/redux#1729, but it's gone inactive (although, @acdlite did just merge in some stuff on the upstream repo just a few hours ago...). Might be good to focus efforts there so we can use the exact same code in both places.
There was a problem hiding this comment.
Yeah. My thought was to wait until these changes have landed, take care of any outstanding issues and let the dust settle before further tinkering.
|
@timdorr Do you think this is ready for another alpha release? |
|
Yeah, I'll do that right now. |
|
|
tests and fixes reduxjs#457
No description provided.