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

React-Redux v6 beta feedback thread #1083

Closed
markerikson opened this Issue Nov 15, 2018 · 34 comments

Comments

@markerikson
Copy link
Contributor

markerikson commented Nov 15, 2018

Opening this as a place to get user feedback on our React-Redux v6 beta releases. We're specifically interested in bug reports and info on performance behavior, but would also appreciate feedback if it's working correctly as well.

To simplify the comment thread a bit: if you've tried it and it works okay, please add a 👍 reaction to this comment. If something broke, leave a 👎 reaction, plus a comment with details, including a link to your project if publicly available.

We'd also be interested in hearing details on:

  • Particularly complex projects that work fine with v6
  • Other libraries in the ecosystem that break due to changes in v6

The current beta is: React-Redux v6.0.0-beta.3

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 15, 2018

Known libraries that break in v6, and tracking issues if available:

@jlongster

This comment has been minimized.

Copy link

jlongster commented Nov 15, 2018

I'll post now since you asked :) There are two things which I've noticed, neither of which I have reproducible test cases for yet:

  • I was on 5.0.6, and when I switched my app into concurrent mode, one of my components would be rendered with stale props. My usage of redux is pretty light and standard: when the app mounts, it calls an action creator which fetches some budget categories, so it renders once with 0 categories. It should load 4 categories in and render with them. For some reason, a small percentage of the time (maybe 15%?) it would render with 4 categories but then immediately render again with 0 categories. If you have any idea how to debug this, I can do it.

Switching to 6.0.0-beta.2 fixes it. I never could get it to render badly.

  • After switching to 6.0.0-beta.2, I ran my React Native app which is not in concurrent mode. I immediately saw another bug: my top-level component fetches the current "prefs" from a local server, and if they exist, it loads the corresponding budget (if they are null, renders a welcome screen). For some reason, the app renders initially with null prefs (as it should) but when the prefs come in later the component is not rerendered with the new ones. Switching back to 5.1.1 fixed it.
@yjimk

This comment has been minimized.

Copy link

yjimk commented Nov 15, 2018

I feel bad leaving a thumbs down, but with a very basic test and not delving too deep, upgrading to 6.0.0-beta.2 didn't work for me. I tried it on two large corporate eCommerce applications.

I'm not altogether surprised though, they are very large and complex applications touched by a wide range of developers and both have significant redux reliance.

Unfortunately I can't share the source with you as it is private code :(

node: 8.11.4
npm: 6.1.0
react: 16.4.2
redux: 3.7.2
react-redux: 6.0.0-beta.2 (previously using 5.1.1 without issue)
react-router-redux: 5.0.0-alpha.9

Application uses SSR including server side redux store hydration

Application 1
Here's the stacktrace at runtime, server started and webpack was able to build without error. Looks like the error is occurring on the server before any client execution happens. Browser window is a blank screen.

Invariant Violation: withRef is removed. To access the wrapped instance, use a ref on the connected component
    at invariant (/Users/x/app/src/node_modules/invariant/invariant.js:40:15)
    at connectAdvanced (/Users/x/app/src/node_modules/react-redux/lib/components/connectAdvanced.js:70:26)
    at connect (/Users/x/app/src/node_modules/react-redux/lib/connect/connect.js:91:12)
    at Module.<anonymous> (/Users/x/app/src/build/webpack:/web/components/TraitWrapper/index.js:77:23)
    at Module../web/components/TraitWrapper/index.js (/Users/x/app/src/build/server.bundle.js:10118:30)
    at __webpack_require__ (/Users/x/app/src/build/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/Users/x/app/src/build/server.bundle.js:5386:72)
    at Module../web/components/PartGroup/index.js (/Users/x/app/src/build/server.bundle.js:5674:30)
    at __webpack_require__ (/Users/x/app/src/build/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/Users/x/app/src/build/server.bundle.js:5709:69)
    at Module../web/components/PartGroupData/index.js (/Users/x/app/src/build/server.bundle.js:5947:30)
    at __webpack_require__ (/Users/x/app/src/build/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/Users/x/app/src/build/server.bundle.js:834:73)
    at Module../web/components/Builder/index.js (/Users/x/app/src/build/server.bundle.js:1116:30)
    at __webpack_require__ (/Users/x/app/src/build/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/Users/x/app/src/build/server.bundle.js:10772:78)

Application 2

Both of the applications are architecture as similarly as possible from the same custom template. Of course there is a little drift as you'd expect from separate dev teams though. Behaviour was a bit different.

Server starts and webpack builds successfully, SSR works correctly, no errors in the console. Client side execution results in an error though. Visiting the page in the browser flashes with the full server side rendered application (looking good for that brief second), then turns to a blank page when client render kicks in.

screen shot 2018-11-16 at 09 43 23

Sorry if there isn't enough context here for you to go on, I hope some of this is of help!

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 15, 2018

@yjimk : that's okay, that's the feedback we're asking for :)

It looks like both of those issues are actually called out in the release notes already:

  • Breaking change: withRef has been removed, and there's a new forwardRef option instead. Your <TraitWrapper> component should be updated accordingly, along with any other components that call connect(mapState, mapDispatch, null, {withRef : true})(MyComponent) or similar.
  • connected-react-router is trying to access the store from old context, which no longer works - see the issue link near the top of this thread.

Hmm. I may be sorta wrong on the last one - you said you're using react-router-redux, not connected-react-router. What is that <ConnectedRouter> component? It looks like what's in the connected-react-router docs.

@jeffchheng

This comment has been minimized.

Copy link

jeffchheng commented Nov 16, 2018

As you noted, upgrading from v5.0.7 to v6.0.0-beta.2 breaks since we're using redux-forms (v7.1.1). Our app is running React v16.6.3.

With erikras/redux-form#4216 + yarn link, and the app compiles and runs, but it'll crash when mounting any component using redux-form:
Uncaught Error: Could not find "store" in either the context or props of "Connect(Form(InjectIntl(ConnectWrapper(RedactedComponentName))))". Either wrap the root component in a <Provider>, or explicitly pass "store" as a prop to "Connect(Form(InjectIntl(ConnectWrapper(RedactedComponentName))))".

I get this error when rendering react-bootstrap's OverlayTrigger with a popover that's connected to the store, also probably an issue with refs?
Uncaught Error: Could not find "store" in the context of "Connect(Component)". Either wrap the root component in a <Provider>, or pass a custom React context provider to <Provider> and the corresponding React context consumer to Connect(Component) in connect options.

Reproduced with this (edited): https://codesandbox.io/s/4xqkxr0377

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 16, 2018

@jeffchheng : interesting. I traced <OverlayTrigger> down into the react-overlays package, and it looks like they do use React.createPortal() internally:

https://github.com/react-bootstrap/react-overlays/blob/70575b07707baeca66c54108d7882772146d09de/src/Portal.js#L79

If you can set up some CodeSandboxes or repos that reproduce these issues, that would certainly help us investigate.

@jeffchheng

This comment has been minimized.

Copy link

jeffchheng commented Nov 16, 2018

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 18, 2018

Related, here's a poll I ran a few months ago asking about people using store as a prop: https://twitter.com/acemarke/status/972266211064139776

@brunolemos

This comment has been minimized.

Copy link

brunolemos commented Nov 18, 2018

This is a known issue, but currently the Context returns both the store and the storeState which always triggers a re-render on components using useContext. While React doesn't fix this api wise, it would be useful to maybe split this Context in two, one returning only the store, like useContext(ReactReduxStoreContext).

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 18, 2018

@brunolemos : Our use of context internally is considered an implementation detail and not part of our public API. Besides, the entire point of context is to pass down data, and the <Provider> doesn't know what pieces of the store state components may be interested in.

We have a bunch of reasons for switching from having individual components subscribing to having just the <Provider> subscribe. I hope to put together an actual post, but until then, see the extended discussion in Reactiflux starting at https://discordapp.com/channels/102860784329052160/103538784460615680/509533992077361153 .

The React team plans to allow function components to bail out of updates - see facebook/react#14110 .

@brunolemos

This comment has been minimized.

Copy link

brunolemos commented Nov 18, 2018

@markerikson got it

@wgao19

This comment has been minimized.

Copy link
Contributor

wgao19 commented Nov 19, 2018

Hey @yjimk @markerikson

That one looks like what I've encountered with react-router-redux. However, this package is no longer maintained, and is advising to switch to connected-react-router. Both of these packages are broken by the new implementation of v6. As the router connectors can no longer grab the store from context.

Here's the open issue for it: connected-react-router: #176.

Meanwhile, on the side, I'm trying to come up with an alignment for connected-react-router. As v6 requires user to provide the context instance, I cannot think of an implementation for it. For connected-react-router to work, it would have to ask the user to pass it the context, which in turn requires that the user provides React-Redux a customized context. Or is there another way around? After all, requiring the end user of all to pass down a context feels like a bit of extra work?

Is there an expected way for related libraries to gain access to the store with v6?

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Nov 19, 2018

Maybe there should be a <LegacyStoreProvider> or something that you can put in a tree that would keep providing context from a <Provider> above to legacy readers.

(Note that it would still eventually break when legacy context is removed.)

@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Nov 19, 2018

Can our Provider not provide both APIs?

@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Nov 19, 2018

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Nov 19, 2018

Can our Provider not provide both APIs?

Note that legacy context creates a cost for the entire app. There are many bailouts that don't work because there is legacy context in the tree. So there should be a way for users to avoid it completely.

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 19, 2018

@wgao19 : my assumption has been that libraries that still want access to the store can use the exported default ReactReduxContext instance to gain access to it. We won't support that officially as part of the public API (just like we never officially supported accessing the store out of legacy context), but if libraries really want to, I don't want to stop them from doing that.

Example:

import {connect, ReactReduxContext} from "react-redux";

class MyComponent extends React.Component {
    render() {
        return (
            <ReactReduxContext.Consumer>
            {({store}) => {
                // do something useful with the store here
            }}
            </ReactReduxContext.Consumer>
        );
    }
}
@cellog

This comment has been minimized.

Copy link
Contributor

cellog commented Nov 19, 2018

We could provide a copy/pastable snippet for users who need store passed through legacy context. Or we could use this time to do PRs to external projects that break? Or both?

@mattkrick mattkrick referenced this issue Nov 20, 2018

Open

migrate away from legacy context API #2475

0 of 1 task complete
@wgao19

This comment has been minimized.

Copy link
Contributor

wgao19 commented Nov 20, 2018

Thanks @markerikson for the hints, it's working!

@evertbouw

This comment has been minimized.

Copy link

evertbouw commented Nov 22, 2018

I was using react-helmet in a connected component that updates a lot. This blew up the stack somehow. I removed react-helmet from that component and now everything works fine.

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 22, 2018

@evertbouw : can you provide a CodeSandox that reproduces the issue, or at least let us know what kind of error was happening?

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 23, 2018

In discussions with Evert on Reactiflux, it sounds if this may be somehow related to react-helmet doing deep equality checks in its sCU: nfl/react-helmet#373 .

No idea how that ties into our beta, though.

@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Nov 23, 2018

@jeffchheng : interesting. I traced <OverlayTrigger> down into the react-overlays package, and it looks like they do use React.createPortal() internally:

This is fixed in react-bootstrap 1.0.0 betas. They are using the older unstable_renderSubtreeIntoContainer in the current 0.32.4 release, which doesn't support the new context API.

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 23, 2018

Aha, so it's a diff between master and latest release. Good to know.

@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Nov 23, 2018

No idea how that ties into our beta, though.

It doesn't. It's their fix to make, not ours.

I think we're close to a full release. Might as well have our release so dependent libraries have a stable release to target. Both connected-react-router and redux-form are peer dep'd correctly to prevent (or at least warn about) installing an incompatible version. Users of those libraries can hold off until they have compatible releases.

I'm thinking an RC on Monday and then get it out that week as long as no one raises any alarms.

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 23, 2018

Part of me really wants to see some more people replying with "YES, I've tried it in our prod app and it works fine!". Not sure how to get more responses, though.

@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Nov 24, 2018

It was the same with 5.0 and 4.0, so I'm not particularly worried. If it's that bad, you just deprecate it on npm and set the latest tag back to 5.1.1. Live to try again another day.

@Jessidhia

This comment has been minimized.

Copy link

Jessidhia commented Nov 24, 2018

It's been running on pixiv.net for a bit over a week now 😇

I had already gotten rid of previous uses of withRef so I dodged that breaking change.

I wrote a tiny reimplementation of connected-react-router that supports only the minimal features we needed to work, and both react-redux and react-router are running @next, so no legacy context.

I haven't tried StrictMode yet but I think I actually got rid of all uses of legacy context with these updates.

@supasate

This comment has been minimized.

Copy link

supasate commented Nov 25, 2018

For connected-react-router, I just published it as @next (v6.0.0-beta.1). It supports both default ReactReduxContext and custom context. I tested with its simple example app and it works fine (details of changes in this PR).

@raunofreiberg

This comment has been minimized.

Copy link

raunofreiberg commented Nov 26, 2018

Went ahead and gave it a shot on a project (it's in production and used quite widely) with "100+ matches in 100+ files" when searching for react-redux. Seemed to work fine after upgrading :)

@eliasthompson

This comment has been minimized.

Copy link

eliasthompson commented Nov 27, 2018

I had an issue with v5 regarding forwarding refs on a personal project. Been using the beta ever since #1000 version and it has worked great. I really appreciate all the work done on upgrading this, and looking forward to the 6.0.0 release.

@gustavopch

This comment has been minimized.

Copy link

gustavopch commented Nov 28, 2018

I've tried 6.0.0-beta.3 and my connected components were re-rendering even when mapStateToProps returned identical (===) props. Is it a bug?

EDIT: Confirmed it's specific to v6. Rolled back to 5.1.1 and those re-renders don't happen.

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 28, 2018

@gustavopch : can you please provide a project repo or CodeSandbox that reproduces the issue? We can't do anything with a short description like that.

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Dec 5, 2018

And v6 has gone final!

https://github.com/reduxjs/react-redux/releases/tag/v6.0.0

I'll close this thread now. If there are any other new issues, please open up a new thread (with repros, please!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment