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

Use Hooks internally (aka 7.0) #1209

Merged
merged 40 commits into from
Apr 9, 2019
Merged

Use Hooks internally (aka 7.0) #1209

merged 40 commits into from
Apr 9, 2019

Conversation

timdorr
Copy link
Member

@timdorr timdorr commented Mar 22, 2019

🔥🔥🔥React-Redux v7 beta is out! 🔥🔥🔥 We've just published our first beta of React-Redux version 7, and it should be the fastest version of React-Redux yet! Please try it out in your production apps, and let us know how well it works for you!

https://github.com/reduxjs/react-redux/releases/tag/v7.0.0-beta.0

We've rewritten connect to use hooks internally, switched back to using direct store subscriptions in components, and we're now using React's "batched updates" API to reduce re-renders. v7 is API-compatible with v6 - the major bump is primarily because we require React 16.8.

We've also added a new batch() API you can import and call in your own code to minimize re-renders with multiple dispatches. connect and batch should work correctly in ReactDOM and React Native, and there's a new alternate entry point to use with alternate React renderers.

We've tested and benchmarked, and are confident this beta is ready for you to try out in your own apps. We've got a lot of users who do things we don't know about, so if you do find an issue, please let us know in this issue.

(the other issue is getting all Big McLargeHuge.)

markerikson and others added 30 commits March 17, 2019 23:07
Added rtl.act() calls around dispatches and other component updates
Added clarification on expected mapState calls in some places
Disabled some no-longer-relevant tests per implementation
Made tests run against React 16.8 by default
Matches state as of v7.0.0-alpha.2
Matches state as of v7.0.0-alpha.3
This prevents a bug with Terser (webpack's default minifier) where the
returned batch function isn't defined due to function inlining.

Matches state as of v7.0.0-alpha.5
We were trying to read contextValue.subscription, even if that
value was null.  Reworked logic to handle cases where the store
came in as a prop.
Yeah, this kills the blame history. Sorry. But it's a lot easier
to figure out what the tests are used for now.
Changed to useLayoutEffect to ensure that the lastWrapperProps ref
is written to synchronously when a render is committed. However,
because React warns about this in SSR, added a check to fall back
to plain useEffect under Node so we don't print warnings.

Also added logic to ensure that if an error is thrown during a
mapState function, but the component is unmounted before it can
render, that the error will still be thrown.  This shouldn't happen
given our top-down subscriptions, but this will help surface the
problem in our tests if we ever break the top-down behavior.
@markerikson
Copy link
Contributor

@shapeshifta78 : see #1177: React-Redux Roadmap: v6, Context, Subscriptions, and Hooks

Basically, the plan is:

  • Make sure the v7 beta is completely stable and good to go
  • Release v7 final
  • Figure out what in the world we're doing regarding a public hooks API

@shapeshifta78
Copy link

@markerikson Thanks for the quick reply ;)

@markerikson
Copy link
Contributor

This goes in a global gitignore file, not a project.
@timdorr
Copy link
Member Author

timdorr commented Apr 3, 2019

That's an enyzme-to-json bug: adriantoine/enzyme-to-json#137

@nperichSYKES
Copy link

I didn't upgrade to 6.0 because I hit some edge case bug that I never managed to figure out (didn't spend much time on it as 5.0 already worked fine). But based on a quick spot check, 7.0 seems to work perfect! I definitely don't get the same problem, whatever it was.

Anyway, 7.0 is working good for me so far on a reasonably complex app.

@markerikson
Copy link
Contributor

Just published 7.0.0-beta.1, with a fix from #1220 that should resolve the "one update behind" issue reported in #1219 .

@kbrandwijk
Copy link

kbrandwijk commented Apr 4, 2019

Using 7.0 in production since the beta release. Just wanted to let you know that I haven't experienced any issues so far! It also 'feels' faster, but I don't have any metrics to support that feeling...

@timdorr
Copy link
Member Author

timdorr commented Apr 4, 2019

One last request before release: Can we move to react-hooks-testing-library? https://www.npmjs.com/package/react-hooks-testing-library

@markerikson
Copy link
Contributor

@timdorr : given that we're not using any actual custom hooks atm, is that lib actually necessary in any way?

@markerikson
Copy link
Contributor

One other thought to throw out there. Given what we've been seeing with v6, it might be worth updating the "Cannot find store" error message to suggest examining build config or something.

@markerikson
Copy link
Contributor

After discussion with Tim, I'm tentatively planning to publish v7 as final early next week if no new issues pop up.

PLEASE KEEP TESTING THE BETA, FOLKS!

@timdorr
Copy link
Member Author

timdorr commented Apr 4, 2019

Oh, my bad. They just extracted testHook out, which we aren't using. Nevermind!

@ghost
Copy link

ghost commented Apr 4, 2019

Just giving some external feedback:

  • Going from v6 to v7 fixed some issues with mapStateToProps being called again with a stale state
  • There is a small breakage for people using rollup for libraries that don't bundle react-dom, as the dependency on react-dom is now explicit due to unstable_batchUpdates. The fix is to have that dependency explicit in rollup.config.js:
{
  plugins: [
    commonjs({
      include: "node_modules/**",
      namedExports: {
          "node_modules/react-dom/index.js": ["unstable_batchedUpdates"]
        }
      })
  ]
}
  • Definitely faster too!

Thanks for the hard work around that release! 💖

@markerikson
Copy link
Contributor

@kelseasy : can you clarify what the build issue is, exactly? And is that suggested tweak for our Rollup config, or someone else's?

@ghost
Copy link

ghost commented Apr 4, 2019

Oh sorry, it was my rollup config that needed more info, that change is already in your rollup config. Which means that every downstream consumer (that isn't bundling react-dom) will have to make that change. Sorry that wasn't clear!

@asundlihardig
Copy link

I installed 7.0.0-beta.1 on two projects in production. They have been running for a couple of days now. No issues so far 🙌🏻

@markerikson
Copy link
Contributor

Well, if there's one thing my experience as a maintainer has taught me...

it's that the only way to guarantee getting feedback is to publish a new major.0 release.

Let's do this.

@markerikson markerikson merged commit fa58572 into master Apr 9, 2019
@reduxjs reduxjs deleted a comment from abumalick Apr 9, 2019
@timdorr timdorr deleted the v7-beta branch April 12, 2019 20:08
webguru7 pushed a commit to webguru7/react-redux that referenced this pull request Dec 8, 2022
* Update React to latest

* Update React peer dependency to 16.8.x

* Initial re-implementation of `connectAdvanced` using hooks

Matches changes from v7.0.0-alpha.1

* Update tests to match v7-alpha.1 behavior

Added rtl.act() calls around dispatches and other component updates
Added clarification on expected mapState calls in some places
Disabled some no-longer-relevant tests per implementation
Made tests run against React 16.8 by default

* adding a react hooks test that fails with v7 alpha

* wrapping store.dispatch with rlt.act, fixes component renders

* reducing hooks test to 2 components

* Fix case where wrapper props changed before store update render

* Mark ReactDOM as global in UMD builds

Matches state as of v7.0.0-alpha.2

* Fix perf problems with out-of-bounds array access

Matches state as of v7.0.0-alpha.3

* Add modules to handle importing batchedUpdates

* Use appropriate batched update API for subscriptions

* Inject unstable_batchedUpdates in default entry point

* Provide an alternate entry point for alternate renderers

Matches state as of v7.0.0-alpha.4

* Remove batch arg from createListenerCollection (reduxjs#1205)

This prevents a bug with Terser (webpack's default minifier) where the
returned batch function isn't defined due to function inlining.

Matches state as of v7.0.0-alpha.5

* Remove older React versions from Travis

* Add comments to connectAdvanced. Many much comments!

* Re-add test for a custom store as a prop

* Fix null pointer exception when store is given as a prop

We were trying to read contextValue.subscription, even if that
value was null.  Reworked logic to handle cases where the store
came in as a prop.

* Ensure wrapper props are passed correctly when forwarding refs

* Add a test to verify subscription passthrough with store-as-prop

* add non-batched tests (reduxjs#1208)

* Force SSR tests to mimic a Node environment

* Restructure connect tests to group by category for easier reading

Yeah, this kills the blame history. Sorry. But it's a lot easier
to figure out what the tests are used for now.

* Clean up dead code in Provider tests

* Add tests to verify errors are thrown for bad mapState functions

* Fix edge cases around saved wrapper props and error handling

Changed to useLayoutEffect to ensure that the lastWrapperProps ref
is written to synchronously when a render is committed. However,
because React warns about this in SSR, added a check to fall back
to plain useEffect under Node so we don't print warnings.

Also added logic to ensure that if an error is thrown during a
mapState function, but the component is unmounted before it can
render, that the error will still be thrown.  This shouldn't happen
given our top-down subscriptions, but this will help surface the
problem in our tests if we ever break the top-down behavior.

* Formatting

* Add a test to verify no errors are printed in SSR usage

* Ignore .idea/

* 7.0.0-beta.0

* Updated outdated SSR-test (dispatch in ancestors) (reduxjs#1213)

* Added test for injecting dynamic reducers on client and server (reduxjs#1211)

* Remove WebStorm gitignore

This goes in a global gitignore file, not a project.

* [FIX]: reduxjs#1219 Save references before update (reduxjs#1220)

* Re-ignore .idea/

* 7.0.0-beta.1

* Update the codecov config to be more forgiving.

* add test to verify that mapStateToProps is always called with latest store state (reduxjs#1215)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet