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

Jump to bottom

fix: react-hot-loader compatibility #1137

Closed
wants to merge 1 commit into from
Closed

fix: react-hot-loader compatibility #1137

wants to merge 1 commit into from

Conversation

theKashey
Copy link

This PR restores historically important ability to hot-reload redux.

The major change here is driven by prettier due to increased length of conditions, nothing more.

theKashey
theKashey commented Dec 20, 2018
View reviewed changes
src/components/connectAdvanced.js
if (store !== lastStore) {
if (
store !== lastStore ||
lastSelectorFactoryOptions !== selectorFactoryOptions
Copy link
Author

@theKashey theKashey Dec 20, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

factory could change, not only store

theKashey
theKashey commented Dec 20, 2018
View reviewed changes
src/components/connectAdvanced.js
lastChildProps = childProps
lastForwardRef = forwardRef
lastComponent = WrappedComponent
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

component could change. Don't use component from a local scope.

theKashey
theKashey commented Dec 20, 2018
View reviewed changes
src/components/connectAdvanced.js
@@ -182,7 +200,14 @@ export default function connectAdvanced(
)
this.selectDerivedProps = makeDerivedPropsSelector()
this.selectChildElement = makeChildElementSelector()
this.renderWrappedComponent = this.renderWrappedComponent.bind(this)
this.indirectRenderWrappedComponent = this.indirectRenderWrappedComponent.bind(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constructor could not be updated on hot-load.

@netlify
Copy link

netlify bot commented Dec 20, 2018

Deploy preview for react-redux-docs ready!

Built with commit c19b6d5

https://deploy-preview-1137--react-redux-docs.netlify.com

theKashey
theKashey commented Dec 20, 2018
View reviewed changes
src/components/connectAdvanced.js

indirectRenderWrappedComponent(value) {
// calling renderWrappedComponent on prototype from indirectRenderWrappedComponent bound to `this`
return this.renderWrappedComponent(value)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the main trick - redirect to hot-reloadable prototype level.

This was referenced Dec 21, 2018
Open
Open
@shahzaibkhalid
Copy link

Any update on when this will be merged and released?
Currently with React-Redux v6.0.0, HMR is broken and we reverted back to React-Redux 5.1.1.

@markerikson
Copy link
Contributor

@timdorr : any thoughts on this?

@timdorr
Copy link
Member

timdorr commented Jan 3, 2019

Have we run this through the benchmark suite as a regression check?

@theKashey
Copy link
Author

Any guideline for benchmarking?

@markerikson
Copy link
Contributor

markerikson commented Jan 3, 2019 •
edited

I know you've been playing with a clone of the benchmarks harness repo. Run comparisons of v5, v6, and a build from this branch, and see if there's any meaningful perf differences between v6 and this branch in particular.

@glenjamin
Copy link

Any update on this? As redux was invented to facilitate hot reloading, it was a bit annoying to find out that things aren't working anymore 😁

Anything I can do to help?

@glenjamin
Copy link

Ok, slightly more useful comment from me this time:

Benchmarks show no appreciable difference:

> node ./runBenchmarks.js

Running benchmark stockticker
  react-redux version: 5.1.1
    Checking max FPS... (30 seconds)
    Running trace...    (30 seconds)
  react-redux version: 6.0.0-pr-1137
    Checking max FPS... (30 seconds)
    Running trace...    (30 seconds)
  react-redux version: 6.0.0
    Checking max FPS... (30 seconds)
    Running trace...    (30 seconds)

Results for benchmark stockticker:
┌───────────────┬─────────┬───────────┬───────────┬──────────┬─────────────────────────────────────────────────────────────────────────────────┐
│ Version       │ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                                                                      │
├───────────────┼─────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────────────┤
│ 5.1.1         │ 39.23   │ 10438.44  │ 15240.20  │ 2555.41  │ 1,28,26,28,33,41,43,36,42,41,46,43,42,26,44,41,44,43,42,44,41,40,39,40,44,41,42 │
├───────────────┼─────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────────────┤
│ 6.0.0         │ 32.64   │ 13067.74  │ 13107.78  │ 2124.42  │ 1,33,38,37,38,36,37,34,33,31,32,33,32,31,30,32,31,29,31,29,30,32,29             │
├───────────────┼─────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────────────┤
│ 6.0.0-pr-1137 │ 30.86   │ 13023.47  │ 13117.75  │ 2108.18  │ 1,35,36,35,32,33,34,32,31,30,31,28,30,31,29,30,29,30,28,29,28,30,28             │
└───────────────┴─────────┴───────────┴───────────┴──────────┴─────────────────────────────────────────────────────────────────────────────────┘
Running benchmark tree-view
  react-redux version: 5.1.1
    Checking max FPS... (30 seconds)
    Running trace...    (30 seconds)
  react-redux version: 6.0.0-pr-1137
    Checking max FPS... (30 seconds)
    Running trace...    (30 seconds)
  react-redux version: 6.0.0
    Checking max FPS... (30 seconds)
    Running trace...    (30 seconds)

Results for benchmark tree-view:
┌───────────────┬─────────┬───────────┬───────────┬──────────┬────────────────────────────────────────────────────────────────────────────────────────┐
│ Version       │ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                                                                             │
├───────────────┼─────────┼───────────┼───────────┼──────────┼────────────────────────────────────────────────────────────────────────────────────────┤
│ 5.1.1         │ 51.81   │ 8954.33   │ 7512.14   │ 586.76   │ 1,53,52,51,55,47,49,51,49,54,55,53,47,49,54,50,51,53,57,51,57,49,50,51,53,56,50,52     │
├───────────────┼─────────┼───────────┼───────────┼──────────┼────────────────────────────────────────────────────────────────────────────────────────┤
│ 6.0.0         │ 49.93   │ 12609.78  │ 6204.79   │ 465.91   │ 41,54,46,49,54,44,47,39,51,47,54,52,45,56,41,54,52,48,51,46,54,51,53,56,49,52,53,51,49 │
├───────────────┼─────────┼───────────┼───────────┼──────────┼────────────────────────────────────────────────────────────────────────────────────────┤
│ 6.0.0-pr-1137 │ 49.81   │ 12485.60  │ 6156.48   │ 458.79   │ 1,50,49,42,56,53,44,51,46,51,46,52,51,53,44,54,48,54,50,51,53,47,52,55,47,51,48,47     │
└───────────────┴─────────┴───────────┴───────────┴──────────┴────────────────────────────────────────────────────────────────────────────────────────┘
Running benchmark twitter-lite
  react-redux version: 5.1.1
    Checking max FPS... (30 seconds)
    Running trace...    (30 seconds)
  react-redux version: 6.0.0-pr-1137
    Checking max FPS... (30 seconds)
    Running trace...    (30 seconds)
  react-redux version: 6.0.0
    Checking max FPS... (30 seconds)
    Running trace...    (30 seconds)

Results for benchmark twitter-lite:
┌───────────────┬─────────┬───────────┬───────────┬──────────┬─────────────────────────────────────────────────────────────────────────────────────┐
│ Version       │ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                                                                          │
├───────────────┼─────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────────────────┤
│ 5.1.1         │ 48.15   │ 20598.51  │ 3726.63   │ 377.77   │ 57,58,60,59,60,59,60,58,57,56,54,58,53,48,42,52,50,47,52,47,43,40,35,34,33,31,28,26 │
├───────────────┼─────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────────────────┤
│ 6.0.0         │ 38.38   │ 22760.83  │ 2855.39   │ 321.06   │ 56,59,60,59,60,57,56,59,58,47,45,36,32,33,29,27,26,27,24,21,23,21,22,21,19          │
├───────────────┼─────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────────────────┤
│ 6.0.0-pr-1137 │ 38.95   │ 22354.64  │ 2884.74   │ 310.99   │ 57,59,60,59,58,54,59,49,51,48,43,37,33,31,30,27,29,24,23,21,22,21,19                │
└───────────────┴─────────┴───────────┴───────────┴──────────┴─────────────────────────────────────────────────────────────────────────────────────┘

@markerikson
Copy link
Contributor

Spent the afternoon poking at the benchmarks setup. Thanks to @glenjamin for helping with some upgrades there.

The actual FPS numbers fluctuate from run to run, but yeah, I don't see any appreciable indication that this makes things any slower.

@markerikson
Copy link
Contributor

@theKashey : I rebased this branch against master, but it doesn't look like I can force-push it to your repo atm.

I'm going to close this PR, push my branch, and create a new PR from that.

@markerikson markerikson mentioned this pull request Jan 20, 2019
Merged
@markerikson markerikson mentioned this pull request Feb 3, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@theKashey @shahzaibkhalid @markerikson @timdorr @glenjamin