Moved react-dom to dependencies

[mikko-tormala]

With the inclusion of { unstable_batchedUpdates } from 'react-dom' the react-dom package is now a dependency, as opposed to a devDependency.

Problem:
When installing with pnpm recursive install the react-dom package is not installed as a dependency for the react-redux package and builds using webpack will fail as the package is not available.

Expected:
react-dom should be a dependency.

Workaround until fixed:
Using pnpm hooks (https://pnpm.js.org/docs/en/hooks.html), the following pnpmfile.js will fix the problem by moving the dependency programmatically:

module.exports = {
  hooks: {
    readPackage
  }
}

function readPackage (pkg, context) {
  // react-redux@7: Move react-dom into from devDependencies to dependencies
  if (pkg.name === 'react-redux' && pkg.version.startsWith('7.')) {
    pkg.dependencies['react-dom'] = pkg.devDependencies['react-dom'];
    delete pkg.devDependencies['react-dom'];
    context.log(`react-redux@${pkg.version}: Moved react-dom from devDependencies -> dependencies`);
  }
  return pkg;
}

[netlify]

Deploy preview for react-redux-docs ready!

Built with commit 1989033

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

[markerikson]

Nope, it's not a direct dependency, because it can be used with React Native instead, or even without ReactDOM or React Native if you use the alternate entry point.

[mikko-tormala]

@markerikson while that might be the case with manually using code from this repo, the npm installed package is dependent on react-dom.

Line 2 of the dist/react-redux.js from the react-dom@7.0.2 npm installed package (notice the last require):

typeof exports === 'object' && typeof module !== 'undefined' ? 
factory(exports, require('react'), require('redux'), require('react-dom')) :

Because of that require('react-dom'), every single project that uses react-redux via npm install will require react-dom to be installed separately.

Have a look at this example repo that I made to showcase the problem:
https://github.com/mikko-tormala/redux-react-dependency-test

[markerikson]

No, that file is the UMD standalone build. It's only meant for use in web environments, and it's also not the file a bundler will pick up.

Webpack and other bundlers will read index.js and pull in the ReactDOM batching. Metro, the RN bundler, will pull in the RN batching because of the .native.js extension.

[mikko-tormala]

Why are jest and webpack then both throwing errors in this example repo of the problem:
https://github.com/mikko-tormala/redux-react-dependency-test
Both start working as soon as npm install react-dom is run.

Dependencies:

"dependencies": {
  "react": "^16.8.6",
  "react-redux": "^7.0.2",
  "react-test-renderer": "^16.8.6",
  "redux": "^4.0.1"
},
"devDependencies": {
  "@babel/core": "^7.4.3",
  "@babel/preset-env": "^7.4.3",
  "@babel/preset-react": "^7.0.0",
  "babel-jest": "^24.7.1",
  "babel-loader": "^8.0.5",
  "jest": "^24.7.1",
  "jest-cli": "^24.7.1",
  "webpack": "^4.30.0",
  "webpack-cli": "^3.3.1"
}

index.test.js:

import { connect } from 'react-redux'

describe('Should not throw an error on import', () => {
  it('Succeeded', () => {
    expect(true).toBe(true);
  })
});

npm run test output:

> react-redux-dependency-test@1.0.0 test /Work/open-source/redux-react-dependency-test
> jest

 FAIL  ./index.test.js
  ● Test suite failed to run

    Cannot find module 'react-dom' from 'reactBatchedUpdates.js'

    However, Jest was able to find:
    	'utils/reactBatchedUpdates.js'
    	'utils/reactBatchedUpdates.native.js'

    You might want to include a file extension in your import, or update your 'moduleFileExtensions', which is currently ['js', 'json', 'jsx', 'ts', 'tsx', 'node'].

    See https://jestjs.io/docs/en/configuration#modulefileextensions-array-string

      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:229:17)
      at Object.<anonymous> (node_modules/react-redux/lib/utils/reactBatchedUpdates.js:6:17)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        1.807s

[markerikson]

@mikko-tormala : right. React-Redux does need ReactDOM, in a web environment. However, because React-Redux does not need ReactDOM in other environments, ReactDOM should not be a direct dependency.

[mikko-tormala]

@markerikson Webpack will fail if react-dom is not in node_modules/
To me, that indicates dependency.

Sure,alternate-renderers.js offers a way to use it without react-dom, but it's already in the name of that file alternate that says it's not the primary use case for this package.

The fact that one can use parts of this package without needing react-dom doesn't mitigate the fact that the file marked as the main entry point for this package ((lib|es|dist|src)/index.js) depends on react-dom. Because of that it should be moved into dependencies.

[markerikson]

@mikko-tormala : simply put: React-Redux can be used in multiple environments. React Native is a valid alternate environment. In the RN environment, we do not rely on ReactDOM. Therefore, there is not a direct dependency. If anything, RD and RN are both peer deps, same as always.

[mikko-tormala]

@markerikson If package 1 doesn't work in production without package 2, then package 1 is dependent on package 2.

If there are any use cases where a package breaks in production because of a missing dependency, then that dependency should be added in.

Saying it works for some users in some cases is not the right way to do dependency management. If a package breaks for some users, it should be treated as if it breaks for all users.

I fixed this problem programmatically, but I shouldn't have to.

[timdorr]

Unfortunately, this is a limitation of the npm dependency system. What we actually have is a transient dependency on react-dom. It is a dependency if react-native isn't a dependency. npm doesn't have any way to represent this currently.

[markerikson]

Yup. The closest we could do would be to declare both react-dom and react-native as peer dependencies, but then everyone building web apps would get warnings about RN not being installed, and vice versa.

Realistically, anyone building a web app will have RD already installed (via a CRA project setup or otherwise), and anyone building an RN app will have RN already installed. So, everything just works fine as expected in those situations.

It looks like the only reason you're running into any kind of an issue here is that A) you're using a lesser-known package manager that works differently, and B) you're running a command that (I assume) tries to recursively figure out what deps to install with a single call. And honestly, if you're building a web app, then you should able to add in ReactDOM yourself anyway as part of the app setup process.

[mikko-tormala]

Saying it's a limitation of the npm dependency system is not really accurate, as this package is using it in a way that's not exactly kosher: There are arguably two different packages with different entry points and dependencies baked into one.

I understand the hesitation to shift the react-dom package from devDependencies to dependencies, but the way it is right now is breaking it for users (well, at least one user).

Theoretically, the alternative would be to break the package into two separate packages going forward. One that requires react-dom and one that doesn't. i.e. react-redux-native, react-redux-dom, etc. Or, as the React Native userbase is likely much smaller than the ReactDOM userbase, maybe just shift the React Native usecase into its own package and keep the ReactDOM version as react-redux.

Hypothetically, let's say react-dom was moved into dependencies, semver was bumped to 8.*, and a new react-redux-native package was also made available: React Native users that are observant would notice to switch, and legacy projects / users who are unobservant that there is a separate package for RN would not have any problems either. Nothing would break for React Native users even if ReactDOM was added in as a dependency. Their build artefacts might get padded by some extra kbs, but webpack wouldn't build it into bundles (unless they use a weird config setup, and even then nothing would reference and use ReactDOM) and no harm would be done.

[mikko-tormala]

@markerikson

It looks like the only reason you're running into any kind of an issue here is that A) you're using a lesser-known package manager that works differently, and B) you're running a command that (I assume) tries to recursively figure out what deps to install with a single call. And honestly, if you're building a web app, then you should able to add in ReactDOM yourself anyway as part of the app setup process.

A) This is correct and there's an easy solve I have in place for the problem I now have with it, but it doesn't change the fact that it's a dependency that is required by this package.
B) I have react-dom as a dependency in my project as you suspect, but the way pnpm works is that instead of a flat node-modules structure, each dependency of a project is installed in a way that each package only has access to its own listed dependencies. (Which is how it should be IMO)