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

Theia builds are weirdly broken with npm 5.3.0 + yarn 1.1.0 #4539

Closed
akosyakov opened this issue Sep 25, 2017 · 13 comments · Fixed by #4687
Closed

Theia builds are weirdly broken with npm 5.3.0 + yarn 1.1.0 #4539

akosyakov opened this issue Sep 25, 2017 · 13 comments · Fixed by #4687

Comments

@akosyakov
Copy link

Theia builds are failing with weird errors from tslint with npm 5.3.0 + yarn 1.1.0: https://travis-ci.org/theia-ide/theia/jobs/279443045

With yarn 1.0.2 it is fine: https://travis-ci.org/theia-ide/theia/jobs/279458465

@akosyakov
Copy link
Author

Tried the latest npm 5.4.2 + yarn 1.1.0 the same issue.

@jktravis
Copy link

jktravis commented Sep 25, 2017

Might be related, I upgraded to 1.1.0 this morning, and now, in the browser of my React project, I'm getting this error

Uncaught Error: addComponentAsRefTo(...): Only a ReactOwner can have refs. You might be adding a ref to a component that was not created inside a component's `render` method, or you have multiple copies of React loaded (details: https://fb.me/react-refs-must-have-owner).
    at invariant (invariant.js:44)
    at Object.addComponentAsRefTo (ReactOwner.js:68)
    at attachRef (ReactRef.js:23)
    at Object.ReactRef.attachRefs (ReactRef.js:42)
    at ReactCompositeComponentWrapper.attachRefs (ReactReconciler.js:23)
    at CallbackQueue.notifyAll (CallbackQueue.js:76)
    at ReactReconcileTransaction.close (ReactReconcileTransaction.js:80)
    at ReactReconcileTransaction.closeAll (Transaction.js:209)
    at ReactReconcileTransaction.perform (Transaction.js:156)
    at batchedMountComponentIntoNode (ReactMount.js:126)

I presume this is yarn somehow adding a different copy of a dependency with 1.1.0 that it's not doing in 1.0.2.

It should be noted that the yarn version is the only thing I'm changing.

@hieuhlc
Copy link

hieuhlc commented Sep 26, 2017

npm 3.10.10 + yarn 1.1.0, same issue here

@BYK
Copy link
Member

BYK commented Sep 26, 2017

This is most probably after #4478. I'd say look into your dependency definitions and peer dependency definitions to make sure you are using peerDependencies where you actually want to enforce the same instance of a dependency instead of dependencies or dev dependencies.

If you are sure your configuration is correct, please post a minimal reproduction case here so others can verify and start working on a possible fix. That "others" can be you if you ever fancied working in a package manager code base :)

@hexa00
Copy link

hexa00 commented Sep 26, 2017

After some digging I found out that the typescript peerDependency of tsutils is realised in its node_modules so with

yarn 1.1.0 I get:

tsutils/node/modules/typescript with version 2.4.1 (should be ^2.5.2)

While with 1.0.2 I don't get this directory there.

Seems to me this should not happen since typescript is a peerDependency and should not be realised in tsutils's node_modules right?

@hexa00
Copy link

hexa00 commented Sep 26, 2017

I made a minimalist reproducer project here: https://github.com/hexa00/repro-tslint-yarn

If anyone wants to take a look ?

Thanks!

@devoto13
Copy link
Contributor

I've run repro locally with Yarn 1.1.0 and I got same results. Furthermore yarn list reports different tree than node_modules folder has. See yarn list output in this gist.

Note duplicated typescript@2.4.1 as a dependency of typedoc@0.8.0.
Note that typescript is not listed under tsutils in yarn list output, but it is present in node_modules folder (see README).

@ggsjyoon
Copy link

Upgraded to yarn 1.1.0 yesterday and having exactly the same issue as @jktravis'

@RafaPolit
Copy link

We are facing the same issue using completely different packages as the ones listed: for us react-widgets is throwing this same error.

Also, as a further info, if you just avoid yarn and use npm install, everything works correctly.

Thanks for looking into this.

@gabrielhpugliese
Copy link

I can confirm that 1.0.1 and 1.0.2 are fine but 1.1.0 and 1.2.0 contains what @jktravis showed.

@devoto13
Copy link
Contributor

devoto13 commented Oct 10, 2017

@BYK Any chance you can look into this? It feels like fix for peer dependencies in 1.1.0 introduced another regression. Simple repro is in #4539 (comment) and my findings are in the next comment. Behavior in 1.2.0 is same as behavior described for 1.1.0.

PS Duplicated peer dependencies are likely a root cause of the problem described in the original post.

@BYK
Copy link
Member

BYK commented Oct 11, 2017

@devoto13 thanks for the follow-up. I was curious if 1.2.0 would fix this one but apparently not. I'll look into the comments and try to understand what is happening here.

@BYK
Copy link
Member

BYK commented Oct 11, 2017

Alright, I did some investigation and there indeed seems to be a bug: Yarn prefers resolving peer dependencies from the closest level but it doesn't verify if they are under the same tree with the current implementation. This causes the current, weird resolution due to following:

  • typedoc lists typescript@2.4.1 as a dependency, putting typescript@2.4.1 to level 1
  • tslint depends on tsutils, which is also at level 1 but in a different tree
  • tsutils then depends on typescript as a peer dependency
  • Yarn resolves typescript from the top level first for this, which gives a level distance of 1
  • Yarn then looks for a closer match, which it finds at level 1 from typedoc, yielding to an "ideal", 0-level-distance peer dependency
  • The problem is, these are under completely different trees so it causes duplication and other issues :(

A fix is on the way.

BYK added a commit that referenced this issue Oct 11, 2017
…t trees

**Summary**

Fixes #4539. Yarn was resolving peer dependencies from the closest level where the peer dependency was requested
but it was not checking if the peer dependency was in the same subtree. This was causing incorrect
peer dependency resolutions and package duplication when  an unrelated subtree has a depedency
satisfying the required peer dependency at the same tree level.

**Test plan**

Manually verified. Autmated tests coming.
@BYK BYK closed this as completed in #4687 Oct 11, 2017
BYK added a commit that referenced this issue Oct 11, 2017
#4687)

**Summary**

Fixes #4539. Yarn was resolving peer dependencies from the closest level where the peer dependency was requested
but it was not checking if the peer dependency was in the same subtree. This was causing incorrect
peer dependency resolutions and package duplication when  an unrelated subtree has a depedency
satisfying the required peer dependency at the same tree level.

**Test plan**

Added new install integration test that fails without the fix.
joaolucasl pushed a commit to joaolucasl/yarn that referenced this issue Oct 27, 2017
yarnpkg#4687)

**Summary**

Fixes yarnpkg#4539. Yarn was resolving peer dependencies from the closest level where the peer dependency was requested
but it was not checking if the peer dependency was in the same subtree. This was causing incorrect
peer dependency resolutions and package duplication when  an unrelated subtree has a depedency
satisfying the required peer dependency at the same tree level.

**Test plan**

Added new install integration test that fails without the fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants