CLI: Add peer dependency on react

[bryanjtc]

Issue: #20537

What I did

Added react peer dependencies

How to test

• Is this testable with Jest or Chromatic screenshots?
• Does this need a new example in the kitchen sink apps?
• Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

[IanVS]

Thanks!

[IanVS]

@bryanjtc actually, would you mind making the same change in cli-sb as well?

[bryanjtc]

@IanVS Done

[ndelangen]

I think it would be better to change this code:

storybook/code/lib/cli/src/ensure-react-peer-deps.ts

Lines 5 to 8 in 34d287e

	try {
	require.resolve('react');
	require.resolve('react-dom');
	} catch (e) {

To instead check the contents of package.json.. As-is defeats the purpose AFAICS, because having a peerDependency, ensures it's installed, then we check if it's installed?

We should read package.json, and check if react is defined in either devDependencies, dependencies or peerDependencies.

Please correct me if I'm wrong, but those 2 lines are the reason we've added the peerDependencies in lib/cli?

[ndelangen]

@valentinpalkovic I wanted to bring this review to your attention, because git tells me you wrote the code I highlighted. It's totally possible I'm missing some context. Let me know if you'd like to discuss tomorrow?

[valentinpalkovic]

As-is defeats the purpose AFAICS, because having a peerDependency, ensures it's installed

@ndelangen This is not the case for all package managers, is it? Yarn, pnpm and npm sometimes (configuration dependent) does not automatically install peer dependencies for you. So this logic is still necessary to ensure a valuable user experience, because peer deps warnings are often ignored.

I don‘t understand, though, why we should read the package.jsons content rather than using require.resolve. Can you explain it to me, maybe I miss something.

[ndelangen]

In order to do require.resolve() we have to have a dependency on it. (peer or normal).
If we do that, we can assume we'll detect the version we depend on, and thus the detection is pointless.

I'm aware some package managers will not install the peer-dependency, but some do. And it seems with modern package managers this is the way. I think we'd be wise to simply assume that's how things are.

Looking into package.json instead has 2 major differences:

• It's pure file-system base,
  meaning no command to run, no package manager to invoke, no node process, no resolution process, no version of node to worry about.
• it requires less context,
  meaning it will not require the user to have run yarn install prior (or whatever package manager they use).

And it will check what we actually want to check: Does the user have a direct dependency on react and react-dom in their package.json?

Let's say we run require.resolve() in a setting where the user has a package manager that does not install peerDependencies by default, but the user has a direct dependency. But the user has not ran the install yet.. require.resolve fails! = a false positive

Let's say we run require.resolve() in a setting where the user has a package manager that DOES install peerDependencies.
The user has no direct dependency on react. Maybe it's hoisted, maybe it isn't, who knows? require.resolve() will succeed though, because our package has a dependency on it, therefor it's going to be available. But there's no way to know if the user does indeed have a direct dependency on react added. We're guaranteed to get a negative results, possibly a false negative.

[valentinpalkovic]

In order to do require.resolve() we have to have a dependency on it. (peer or normal).
If we do that, we can assume we'll detect the version we depend on, and thus the detection is pointless.

Only if you assume that peer-deps are automatically installed. But the error that React cannot be resolved is thrown for customers who are using package managers, which do not install peer-deps. Namely for the package managers' yarn and yarn berry. You're right that this check will never error in an environment where pnpm and npm (version 3 through 6 though does not install peer-deps automatically) are used because they take care of throwing an error if peer-deps are not installed correctly.

It's pure file-system base,
meaning no command to run, no package manager to invoke, no node process, no resolution process, no version of node to worry about.

This argument is valid. I would like to add the argument, that require.resolve will not work in an ESM environment, where we would have to call import.meta.resolve() instead and introduce a logic to figure out, in which environment the code is running (CommonJS vs ESM)

meaning it will not require the user to have run yarn install prior (or whatever package manager they use).

I don't get this point TBH. Running storybook dev should never work if dependencies are not installed, should it? So dependencies have to be installed to make Storybook work. Can you elaborate on this argument further?

Let's say we run require.resolve() in a setting where the user has a package manager that does not install peerDependencies by default, but the user has a direct dependency. But the user has not ran the install yet.. require.resolve fails! = a false positive

Same here. Storybook cannot be started if dependencies are not installed.

Let's say we run require.resolve() in a setting where the user has a package manager that DOES install peerDependencies. The user has no direct dependency on react. Maybe it's hoisted, maybe it isn't, who knows?

Is hoisting an issue? Why should it be? We want to ensure, that react and react-dom is resolvable. Why do we care in which kind of manner this happens?

require.resolve() will succeed though, because our package has a dependency on it, therefore it's going to be available. But there's no way to know if the user does indeed have a direct dependency on react added.

Is this important, though? Of course, the correct way would be to install react and react-dom. But maybe the costumer just does not want to use npm or pnpm to not be that strict about peer-deps and the packages are hoisted due to different reasons. We don't know the setup of each individual project. We only want to ensure that an import on react and react-dom will not fail.

I honestly do not like the approach of scanning the package.json, because project structures might be complex. Is it always easy to say, that it is enough to scan the root's package.json? Can we be 100% sure, that the execution context is on the same level as the package.json is placed? I am not sure about that.

Either way, as @IanVS has mentioned in #20454 (comment) we should move the check where it is really needed (@storybook/addon-essentials, @storybook/addon-interactions, @storybook/addon-links, @storybook/angular, @storybook/testing-library). But this would mean that we could revert #20459 (because technically the peer-dependency declaration is not necessary here) and this PR would be obsolete. The react/react-dom check was placed into the cli package though, because it is easier to have it here.

[ndelangen]

I chatted with @valentinpalkovic and we concluded to move the core checking for react-dep to addon-docs.
That should remove the need for lib/cli to depend on react/react-dom.

I'll have to close this PR, and also revert some work from you @IanVS, but it will make a lot more sense, in the end, I think.

[ndelangen]

@bryanjtc thank you for contributing, it's truly appreciated. I'm sorry this PR wasn't merged, certainly not because anything you could have foreseen. I hope to see more contributions from you 🙏

[IanVS]

Thanks for the discussion @valentinpalkovic and @ndelangen. FWIW I agree with your decision to move this check to addon-docs. It has the side-benefit of allowing projects without docs to avoid installing react, if that is their wish.