reduxjs / react-redux

Official React bindings for Redux
https://react-redux.js.org
MIT License
23.37k stars 3.37k forks source link

Context with store has been broken since 6.0.0 #1645

Closed andrew-aladev closed 4 years ago

andrew-aladev commented 4 years ago
import React, { Component } from 'react';
import ReactDOM from 'react-dom';
import PropTypes from 'prop-types';
import { createStore } from 'redux';
import { connect, Provider } from 'react-redux';

const store = createStore(
  (state) => state,
  { key: 'value' },
);

class App extends Component {
  static contextTypes = {
    store: PropTypes.object.isRequired,
  };

  constructor(props, context) {
    super(props, context)
    console.log(context)
  }

  render() {
    return null
  }
}

const CApp = connect()(App);

ReactDOM.render(
  <Provider store={store}>
    <CApp />
  </Provider>,
  document.getElementById('root')
);

I think this code is legal for any react and redux versions, but it is limited to react-redux v5.x only. react-redux >= v6.0.0 is not able to provide store, it is always undefined.

timdorr commented 4 years ago

We switched from the legacy context API in that version to the new createContext API. The legacy context API is deprecated and will be removed in a future version of React. You can read about it in the release notes for that version.

andrew-aladev commented 4 years ago

@timdorr Why you have removed legacy context functionality instead of mark it as deprecated for several releases? Can you imagine the amount of 1-2 year old react applications that become broken today?

timdorr commented 4 years ago

This is only an internal change. No applications using our APIs broke. You shouldn't be using the Redux context yourself.

andrew-aladev commented 4 years ago

@timdorr This was not the internal change, we can open v5.1.2 docs/api/Provider and provide the following information:

The makes the Redux store available to any nested components that have been wrapped in the connect() function.

Note: If you really need to, you can manually pass store as a prop to a connected component, but we only recommend to do this for stubbing store in unit tests, or in non-fully-React codebases. Normally, you should just use .

It means that all connected react components were able to access store using react context api. This was public decision of @sveinpg and @maecapozzi (from git history).

What was the problem to mark this functionality as deprecated but working in v6.x releases?

timdorr commented 4 years ago

You can't use both context APIs at the same time. Provider in 6+ and 5.x are two very different components, so they don't overlap or allow legacy usage along with modern usage.

Regardless, we never documented accessing the store this way. You are using undocumented functionality, so there is no contract that we have to maintain. We have maintained connect compatibility, which is the primary way you should be interacting with Redux. We also now have official documentation on how to directly access the store, even though you really shouldn't: https://react-redux.js.org/using-react-redux/accessing-store

andrew-aladev commented 4 years ago

@timdorr Sorry, you can't understand what i am talking about.

The only one legal (and possible) way to receive store in v5.x was store from context. This method has been clearly documented in v5.x.

The makes the Redux store available to any nested components that have been wrapped in the connect() function.

Note: If you really need to, you can manually pass store as a prop to a connected component, but we only recommend to do this for stubbing store in unit tests, or in non-fully-React codebases. Normally, you should just use .

Today you are trying to say that first sentence about Redux store available to any nested components never existed, store was not available in nested components and authors of react-redux removed just internal functionality. It means that all packages and applications depends on react-redux v5.x and tries to access store has been broken by design.

Guys, if you are reading this issue, please try to avoid react-redux dependency in your applications. Authors of this package are not going to provide public api, they may always consider it as internal api and remove it without any notice (like deprecated).

markerikson commented 4 years ago

@andrew-aladev You're very wrong here - nothing in that last comment was correct at all.

In v5, there wasn't any public documented way to access the store directly in your own code. <Provider> put the store in legacy context, and connect accessed the store from legacy context. The fact that React-Redux used the legacy context API internally was always well-known, but it was never part of our public API. So yes, anyone who depended on that functionality was risking their own code breaking.

Any app that used React-Redux v5 the way it was intended should be able to upgrade straight from v5 to v7 just by bumping the package version. The only meaningful additional breaking change to the public API between those two versions was the change to the withRef option, replaced by forwardRef. That's because we encapsulated the use of context as an implementation detail that your own code never had to worry about.

I truly have no idea what you're trying to complain about here.

What actual use case are you trying to solve? As Tim said, in v7 we do specifically have a documented and publicly supported way to access the store if necessary, via the useStore() hook.

andrew-aladev commented 4 years ago

The makes the Redux store available to any nested components that have been wrapped in the connect() function.

@markerikson I understood, this sentence means nothing, any public declaration in react-redux can be considered as fluffy internal toy and removed immediately. Developers just don't care, it is fine.

The fact that React-Redux used the legacy context API internally was always well-known, but it was never part of our public API. So yes, anyone who depended on that functionality was risking their own code breaking.

Have you made a research among public packages: how much applications (depending on these packages) may use your not public api (not only store from context)? Have you discussed results of that research with other developers and provided an adequate solution? There were no technical issues with keeping that api as deprecated for a long period of time. But you just removed it and feel fine.

Any app that used React-Redux v5 the way it was intended should be able to upgrade straight from v5 to v7 just by bumping the package version.

I know how this imaginary app looks like:

sphere horse

Real application depends on multiple public and private packages, each package depends on react-redux and redux. You have removed not public api, so people have to upgrade all dependencies related to redux v6 or stay safe with v5. Some packages depends on v5 only and not compatible with v6. Some packages depends on v6+ only and not compatible with v5.

90% of developers won't take a risk and force using abandoned and outdated package versions. Can you imagine the price of your decision?

What actual use case are you trying to solve? As Tim said, in v7 we do specifically have a documented and publicly supported way to access the store if necessary, via the useStore() hook.

You shouldn't worry about use case I am trying to solve. You may worry about project management, how to make real world applications movement from version to version easier. But i am sure that you just don't care.

markerikson commented 4 years ago

Yes, I do care about what use cases you're trying to solve, because I don't know why this is even a question in the first place.

Internal APIs are just that: internal.

We did go about seeing what public ecosystem packages might need to be upgraded. Quoting from my post on the version history of React-Redux:

The most common issue was actually with third-party libraries that were trying (and now failing) to access the store directly. Notable examples included connected-react-router, react-redux-firebase, react-redux-subspace, and even redux-form. We couldn't do anything about that, beyond poke maintainers and offer some suggestions on an upgrade path.

There was no feasible way to implement both legacy context and new context at the same time. Besides, this was a major version release, and the entire point of major versions is that they have breaking changes.

Nothing about React-Redux v5 itself changed. If someone wants to stay on v5, that still works as-is. When you upgrade a package as an end user, it's also your responsibility to deal with any other packages that relate to that.

I still truly don't understand what the point of your complaint is here, and frankly you're the only person who's ever raised this particular complaint (and for that matter, almost two years after we released v6!). If you can provide some meaningful, specific concerns that relate to an actual use case you're trying to solve, we can try to offer suggestions, but until then this discussion is effectively a waste of my time.

andrew-aladev commented 4 years ago

There was no feasible way to implement both legacy context and new context at the same time. Besides, this was a major version release, and the entire point of major versions is that they have breaking changes.

Large react was able to keep both legacy and new context apis, than small react-redux should never receive issues with keeping old api as deprecated. If old api is popular it should never be removed at once.

Notable examples included connected-react-router, react-redux-firebase, react-redux-subspace, and even redux-form. We couldn't do anything about that, beyond poke maintainers and offer some suggestions on an upgrade path.

Keeping popular api forever or mark is as deprecated was the right solution.

Nothing about React-Redux v5 itself changed. If someone wants to stay on v5, that still works as-is. When you upgrade a package as an end user, it's also your responsibility to deal with any other packages that relate to that.

You have mentioned react-redux-firebase and redux-form. Do you know how much bugs exists in old versions of that packages. Do you know how much money investor should pay to fix them without migrating to newer versions? Size of patches for outdated packages are comparable with react codebase.

I still truly don't understand what the point of your complaint is here, and frankly you're the only person who's ever raised this particular complaint (and for that matter, almost two years after we released v6!).

I have received new outdated react project that suffers from abandoned and outdated packages. One of the main reason of its sufferings is react-redux v5 and v6 compatibility. I have lost about 3 days of my life to return project back to life.

I am sure that the amount of such projects are significant. You will receive more complains soon: software development will be intensified after COVID. Please be prepared for storm.

markerikson commented 4 years ago

Afraid this is not how software development works.

As I already said, it was impossible at a technical level to support both the legacy and modern context APIs at the same time.

For React-Redux to be usable long-term, we had to release a new version that works with modern context API, which required removing the old API. We did this in a major version. That's how you handle breaking changes.

Yes, upgrading old projects is often painful. That's a fact of life in today's world.

So, one more time:

We changed an internal implementation detail. Anyone who relied on that internal implementation took on the risk of that changing in a way that broke their code. The public API of <Provider store={store}> did not change.

If you would like to contribute to our docs by adding a specific migration guide, I'd happily accept a PR that adds that. But, as already stated, for most people it's as simple as bumping the package version from v5 to v7.

markerikson commented 4 years ago

Oh, one other side note since I just re-read the original post here:

Yes, React-Redux v6 did drop the ability to pass store as a prop to connected components, but we brought that back in v7. That said, your original code snippet did not show that - you were mixing that up with passing store to <Provider>, which has always been supported.

You should really take the time to read my post The History and Implementation of React-Redux, which explains exactly why we've changed the implementation over time, and how.