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

[Feature] Able to forbid / block multiple instances of a package with peer dependencies #4688

Open
2 tasks done
dobesv opened this issue Jul 29, 2022 · 10 comments
Open
2 tasks done
Labels
enhancement New feature or request

Comments

@dobesv
Copy link

dobesv commented Jul 29, 2022

  • I'd be willing to implement this feature (contributing guide)
  • This feature is important to have in this repository; a contrib plugin wouldn't do

Describe the user story

There are certain packages which have global variables that are assumed to be shared throughout an application, and
which also have peer dependencies. For example, styled-components & react-dom. We also have some packages
in our monorepo that have global state.

When a package has peer dependencies, yarn may create multiple copies of that package if the peer dependencies
are resolved differently via different dependency paths.

It seems to be relatively easy to accidentally have multiple copies of styled-components being used
in one application simply by having one dependency on package that has a peer dependency on react-is and another that
does not.

This actually breaks styled-components or any other package that relies on some internal state, but not in an obvious way where you can easily go find the problem. Instead you can just get weird issues with global variables being different in different packages.
It's very confusing and hard to debug.

Yarn also doesn't provide an easy way to figure out why the virtual copies were created, at least not that I have found. So I have to run yarn why and yarn info and try to compare the peer dependencies used in one place versus another.

Describe the solution you'd like

Have a configured whitelist of packages which are allowed to have multiple copies in a workspace. When resolving packages would have resulted in two instances of a package, and the package is not in the whitelist, exit with an error suggesting the user resolve the peer dependencies issues causing the split, or else add the package to the whitelist if they don't mind the duplication.

Describe the drawbacks of your solution

Having additional options does add some complexity to the tool, and if they are rarely used then it could be a waste. If the whitelist is applied by default, many users might end up having to do a bunch of work to bring their dependencies up to date to avoid the new error.

Describe alternatives you've considered

Currently I have added a step to our CI that runs some yarn commands to ensure certain packages that are broken by this virtual package issue have a single instance in the workspace:

            mkdir -p yarn-info
            BAD=
            for PKG in \
              @apollo/client \
              react-dom \
              react-helmet \
              react-i18next \
              react-router \
              styled-components \
              subscriptions-transport-ws \
                ; do
              yarn info --dependents --virtuals --recursive ${PKG} > "yarn-info/${PKG}.log"
              grep -q 'Instances: 1' "yarn-info/${PKG}.log" || BAD="$BAD $PKG"              
            done
            if [[ ! -z "$BAD" ]] ; then
               echo "Packages have multiple instances, please check the dependencies and fix: ${BAD}"
               exit 1
            fi
@dobesv dobesv added the enhancement New feature or request label Jul 29, 2022
@rtsao
Copy link
Contributor

rtsao commented Dec 13, 2022

Agreed this is a major usability hurdle with Yarn.

By convention, peerDependencies are often used in practice to enforce singletons. However, since Yarn will create multiple virtual instances even when peerDependencies are declared, this convention does not work.

This especially problematic with packages that export React Context, as multiple instances will cause React context to break in a way that is not immediately obvious.

Yarn should definitely implement some mechanism to enforce singletons for packages, preferably via some configuration to allow the existing convention of using peerDependencies to work as expected. I realize the current behavior is deemed expected, but the mandated strictness of provided peer dependencies via duplicate virtual instances is often a cure worse than the disease.

Related:

@neongreen
Copy link

How does pnpm handle this, if at all?

@neongreen
Copy link

A slight modification of @dobesv's script to avoid creating a temp folder:

#!/usr/bin/env bash

# Checks that some deps are unique throughout the project, see https://github.com/yarnpkg/berry/issues/4688

SINGLETONS="react react-dom @emotion/react @emotion/styled"

BAD=
for PKG in $SINGLETONS; do
  log=$(yarn info --dependents --virtuals --recursive ${PKG})
  if [[ "$log" != *"Instances: 1"* ]]; then BAD+="\n  - $PKG"; fi
done

if [[ ! -z "$BAD" ]] ; then
  printf "Packages have multiple instances, please check the dependencies and fix: ${BAD}\n"
  echo "To check, run: yarn info --dependents --virtuals --recursive <package>"
  exit 1
fi

@neongreen
Copy link

neongreen commented Jan 2, 2024

Update: I think it doesn't work properly (neither the original script nor mine). I found at least several interesting cases:

Only one version (good), no "Instances" printed at all

{"value":"react@npm:17.0.2","children":{"Version":"17.0.2","Dependencies":[{"descriptor":"loose-envify@npm:^1.1.0","locator":"loose-envify@npm:1.4.0"},{"descriptor":"object-assign@npm:^4.1.1","locator":"object-assign@npm:4.1.1"}]}}

Two versions (bad?), no "Instances" printed at all

{"value":"react@npm:15.7.0","children":{"Version":"15.7.0","Dependencies":[{"descriptor":"create-react-class@npm:^15.6.0","locator":"create-react-class@npm:15.7.0"},{"descriptor":"fbjs@npm:^0.8.9","locator":"fbjs@npm:0.8.18"},{"descriptor":"loose-envify@npm:^1.1.0","locator":"loose-envify@npm:1.4.0"},{"descriptor":"object-assign@npm:^4.1.0","locator":"object-assign@npm:4.1.1"},{"descriptor":"prop-types@npm:^15.5.10","locator":"prop-types@npm:15.8.1"}]}}
{"value":"react@npm:17.0.2","children":{"Version":"17.0.2","Dependencies":[{"descriptor":"loose-envify@npm:^1.1.0","locator":"loose-envify@npm:1.4.0"},{"descriptor":"object-assign@npm:^4.1.1","locator":"object-assign@npm:4.1.1"}]}}

Two or three (?) versions, but one of them is "Instances: 1"

{"value":"@angular-devkit/core@npm:13.3.5","children":{"Instances":1,"Version":"13.3.5","Dependencies":[{"descriptor":"ajv-formats@npm:2.1.1","locator":"ajv-formats@npm:2.1.1"},{"descriptor":"ajv@npm:8.9.0","locator":"ajv@npm:8.9.0"},{"descriptor":"fast-json-stable-stringify@npm:2.1.0","locator":"fast-json-stable-stringify@npm:2.1.0"},{"descriptor":"magic-string@npm:0.25.7","locator":"magic-string@npm:0.25.7"},{"descriptor":"rxjs@npm:6.6.7","locator":"rxjs@npm:6.6.7"},{"descriptor":"source-map@npm:0.7.3","locator":"source-map@npm:0.7.3"}]}}
{"value":"@angular-devkit/core@npm:13.3.6","children":{"Instances":2,"Version":"13.3.6","Dependencies":[{"descriptor":"ajv-formats@npm:2.1.1","locator":"ajv-formats@npm:2.1.1"},{"descriptor":"ajv@npm:8.9.0","locator":"ajv@npm:8.9.0"},{"descriptor":"fast-json-stable-stringify@npm:2.1.0","locator":"fast-json-stable-stringify@npm:2.1.0"},{"descriptor":"magic-string@npm:0.25.7","locator":"magic-string@npm:0.25.7"},{"descriptor":"rxjs@npm:6.6.7","locator":"rxjs@npm:6.6.7"},{"descriptor":"source-map@npm:0.7.3","locator":"source-map@npm:0.7.3"}]}}

@neongreen
Copy link

neongreen commented Jan 2, 2024

Which actually made me realize I don't understand the original request exactly.

@dobesv do you want:

  1. A warning when you have multiple virtual packages for a dependency (due to a peer dependency split), even if they have the same version?
  2. A warning when you have multiple different versions of a dependency (eg. react-16 and react-17), even if each only has one virtual instance?
  3. A warning regardless of whether (1) or (2) happens?

@dobesv
Copy link
Author

dobesv commented Jan 2, 2024

Ultimately, what I want to avoid is having multiple copies of packages like react, styled-components, and so on to be loaded by my application directly or indirectly with different copies of their internal state.

I don't want a warning in these case - I want yarn to exit failure if it would have installed more than one copy of any of those packages.

Since I don't know in advance which packages have state in them, it's safer to use a "whitelist" approach where if I get the error, I can try to resolve the conflict, or verify that the package can be safely used with multiple instances and whitelist it.

It doesn't matter to me whether there are multiple versions of the package installed, or just multiple instances due to the resolution of peer dependencies.

Both cases are probably an error as far as I am concerned.

@neongreen
Copy link

If both cases are an error, then I think you'd have to detect them separately:

#!/usr/bin/env bash

# Checks that some deps are unique throughout the project, see https://github.com/yarnpkg/berry/issues/4688.
#
# Even the same version (but different virtual packages) is not acceptable, because JS packages are initialized once for each *path*.

SINGLETONS="react react-dom react-router @emotion/react @emotion/cache @emotion/styled"

fail=false

echo "Note: don't forget to run 'yarn' before running this script."

for PKG in $SINGLETONS; do
  log_without_virtuals=$(yarn info --dependents --recursive --json "$PKG")
  # If log_without_virtuals has more than one line, the package has multiple versions.
  if [[ $(echo "$log_without_virtuals" | wc -l) -ne 1 ]]; then
    fail=true
    echo
    echo "$PKG has multiple versions:"
    echo "  yarn info --dependents --recursive $PKG"
  fi

  # If log_with_virtuals has Instances and any of the numbers is not 1, the package has multiple virtual instances.
  log_with_virtuals=$(yarn info --dependents --recursive --virtuals --json "$PKG")
  has_multiple_instances=false
  for i in $(echo "$log_with_virtuals" | grep -o '"Instances":[0-9]*' | cut -d: -f2); do
    [[ $i -ne 1 ]] && has_multiple_instances=true
  done
  if [[ $has_multiple_instances == true ]]; then
    fail=true
    echo
    echo "$PKG has multiple virtual instances:"
    echo "  yarn info --dependents --recursive --virtuals $PKG"
  fi
done

if [[ $fail == true ]]; then
  exit 1
else
  echo "All seems good."
fi

@dobesv
Copy link
Author

dobesv commented Jan 2, 2024

🤔 I wonder if yarn constraints could check this somehow. It doesn't look like it has access to the fully resolved dependency set, though.

@dobesv
Copy link
Author

dobesv commented Jan 2, 2024

If both cases are an error, then I think you'd have to detect them separately

Sure, yeah, makes sense. I have some other thing that ensures the dependency versions are consistent between packages so I am fine with the script I made. My goal here isn't to produce a shell script necessarily, that's a workaround. Rather a core yarn feature to help people with this since I imagine it's causing headaches for quite a few people out there using a monorepo with yarn.

@neongreen
Copy link

(Oh, absolutely it should be a feature. I just posted the script because I spend a lot of time hunting down others' workarounds in github issues, so maybe somebody will also be able to steal this one.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants