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

Peer Dependency Not Hoisting Along With Other Package #4446

Closed
ghost opened this issue Sep 13, 2017 · 8 comments · Fixed by #4478
Closed

Peer Dependency Not Hoisting Along With Other Package #4446

ghost opened this issue Sep 13, 2017 · 8 comments · Fixed by #4478
Assignees
Labels

Comments

@ghost
Copy link

ghost commented Sep 13, 2017

Do you want to request a feature or report a bug?

A bug.

What is the current behavior?

I have a project that depends on a build tool, a build tool which exports Gulp tasks.

Package examples can be seen here - https://github.com/destroyerofbuilds/dependency-hoisting

From within the application/node_modules/ directory you will see a package called gulp-tslint:

$ ls node_modules/gulp-tslint/
CHANGELOG.md  index.d.ts  index.js  LICENSE  package.json  README.md  tsconfig.json

gulp-tslint is a dependency of build-tool, the development dependency of the application project.

gulp-tslint has a peer dependency on tslint.

built-tool also lists tslint as a dependency.

However, tslint was not hoisted to the top-level application/node_modules/ directory:

$ ls node_modules/tslint
ls: cannot access 'node_modules/tslint': No such file or directory

Running gulp lint, I observe the following error:

$ $(yarn bin)/gulp lint
module.js:471
    throw err;
    ^

Error: Cannot find module 'tslint'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (C:\Users\hbetts\Workspace\dependency-hoisting\application\node_modules\gulp-tslint\index.js:6:14)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)

That error is thrown by the following require in gulp-tslint - https://github.com/panuhorsmalahti/gulp-tslint/blob/53534f8fc57b405551559ed336be38ebda172bf1/index.js#L6

If the current behavior is a bug, please provide the steps to reproduce.

Please follow the steps in the Steps section - https://github.com/destroyerofbuilds/dependency-hoisting#steps

What is the expected behavior?

I would expect that tslint would be hoisted to the project's top-level node_modules/ directory, along side gulp-tslint.

Please mention your node.js, yarn and operating system version.

$ yarn --verison
yarn install v1.0.2
info No lockfile found.
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
info Lockfile not saved, no dependencies.
Done in 0.10s.

$ node --version
v6.11.3

Operating system is Windows 10.

@ghost
Copy link
Author

ghost commented Sep 13, 2017

I downloaded and installed Yarn 0.27.5 (The last, officially released, version of Yarn).

$ yarn --version
0.27.5

I ran through an installation within the application/ directory and then looked for tslint:

$ ls node_modules/tslint/
bin  CHANGELOG.md  lib  LICENSE  node_modules  package.json  README.md

gulp lint seems to work:

$ gulp lint
[17:01:55] Using gulpfile ~\Workspace\dependency-hoisting\application\gulpfile.js
[17:01:55] Starting 'lint'...
[17:01:55] Finished 'lint' after 321 ms

@BYK
Copy link
Member

BYK commented Sep 13, 2017

Thank you very much for the detailed and clear report! That said this is not a bug, an expected behavior. Your application package does not list tslint as a dependency so there are no guarantees for tslint package, a transient-dependency, to be hoisted to the very top.

If your project relies on tslint, then it should list it as a dependency.

@BYK BYK added the not-a-bug label Sep 13, 2017
@ghost
Copy link
Author

ghost commented Sep 13, 2017

The application project does not directly rely, or depend, on tslint.

gulp-tslint, a direct dependency of build-tool, has a peer dependency on tslint.

build-tool lists both gulp-tslint and tslint as direct dependencies.

I would expect that gulp-tslint and tslint would always be side-by-side in the same node_modules directory, whether that's the top-level node_modules directory, or the node_modules directory under build-tool.

I was hoping Yarn would see the peer dependency gulp-tslint has on tslint and ensure that both were installed side-by-side.

@BYK
Copy link
Member

BYK commented Sep 13, 2017

I was hoping Yarn would see the peer dependency gulp-tslint has on tslint and ensure that both were installed side-by-side.

I think that's fair. That said Yarn's algorithm works to satisfy the node require.resolve algorithm and in this case it looks a bit counter-intuitive. That said if you think about it, as long as build-tool can access tslint and when other packages also list tslint as a peerDependency, they all use the same one, this outcome is "correct".

The application project does not directly rely, or depend, on tslint.

Based on your explanation, it actually seems to be relying on tslint's existence on the top level whcih indicates that it relies on it. If you disagree, I'd be interested in hearing your reasoning.

@dphoebus
Copy link

@destroyerofbuilds I'm right there with you. Yarn 1.0+ is basically unusable at this point for me. Running the install command fails every time complaining of not being able to find some peer dependency. And since it fails and the install command doesn't complete... when I try to install the missing dependency it fails on some other missing dependency causing a sad circle of fail.

> yarn install
yarn install v1.0.2
info No lockfile found.
[1/4] Resolving packages...
[2/4] Fetching packages...
info [email protected]: The platform "win32" is incompatible with this module.
info "[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
warning "[email protected]" has incorrect peer dependency "ajv@>=5.0.0".
warning "[email protected]" has incorrect peer dependency "ajv@>=4.10.0".
warning "@most/[email protected]" has incorrect peer dependency "most@^1.0.1".
warning "[email protected]" has incorrect peer dependency "marked@^0.3.3".
[4/4] Building fresh packages...
[1/7] ⠄ uglifyjs-webpack-plugin
[2/7] ⠄ node-sass
[3/7] ⠄ gifsicle
[4/7] ⠄ mozjpeg
error D:\src\Git_Work\spec1\SpecBuilder.Web.Client\node_modules\node-sass: Command failed.
Exit code: 1
Command: node scripts/install.js
Arguments:
Directory: D:\src\Git_Work\spec1\SpecBuilder.Web.Client\node_modules\node-sass
Output:
module.js:491
    throw err;
    ^

Error: Cannot find module 'request'
    at Function.Module._resolveFilename (module.js:489:15)
    at Function.Module._load (module.js:439:25)
    at Module.require (module.js:517:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (D:\src\Git_Work\spec1\SpecBuilder.Web.Client\node_modules\node-sass\scripts\install.js:10:13)
    at Module._compile (module.js:573:30)
    at Object.Module._extensions..js (module.js:584:10)

So I try to install request module ...

> yarn add request --dev --exact                                                                                                                     
yarn add v1.0.2                                                                                                                                      
info No lockfile found.                                                                                                                              
[1/4] Resolving packages...                                                                                                                          
[2/4] Fetching packages...                                                                                                                           
info [email protected]: The platform "win32" is incompatible with this module.                                                                          
info "[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.                                      
[3/4] Linking dependencies...                                                                                                                        
warning "[email protected]" has incorrect peer dependency "ajv@>=5.0.0".                                                                            
warning "[email protected]" has incorrect peer dependency "ajv@>=4.10.0".                                                                           
warning "@most/[email protected]" has incorrect peer dependency "most@^1.0.1".                                                                         
warning "[email protected]" has incorrect peer dependency "marked@^0.3.3".                                                                       
[4/4] Building fresh packages...                                                                                                                     
[1/7] ⠄ uglifyjs-webpack-plugin                                                                                                                      
[2/7] ⠄ node-sass                                                                                                                                    
[3/7] ⠄ gifsicle                                                                                                                                     
[4/7] ⠄ mozjpeg                                                                                                                                      
error D:\src\Git_Work\spec1\SpecBuilder.Web.Client\node_modules\node-sass: Command failed.                                                           
Exit code: 1                                                                                                                                         
Command: node scripts/install.js                                                                                                                     
Arguments:                                                                                                                                           
Directory: D:\src\Git_Work\spec1\SpecBuilder.Web.Client\node_modules\node-sass                                                                       
Output:                                                                                                                                              
module.js:491                                                                                                                                        
    throw err;                                                                                                                                       
    ^                                                                                                                                                
                                                                                                                                                     
Error: Cannot find module 'extend'                                                                                                                   
    at Function.Module._resolveFilename (module.js:489:15)                                                                                           
    at Function.Module._load (module.js:439:25)                                                                                                      
    at Module.require (module.js:517:17)                                                                                                             
    at require (internal/module.js:11:18)                                                                                                            
    at Object.<anonymous> (D:\src\Git_Work\spec1\SpecBuilder.Web.Client\node_modules\request\index.js:17:29)                                         
    at Module._compile (module.js:573:30)                                                                                                            
    at Object.Module._extensions..js (module.js:584:10)

This is where I go back to using v0.27.5 which works perfectly.

> node .\yarn-0.27.5.js install
yarn install v0.27.5
info No lockfile found.
[1/4] Resolving packages...
[2/4] Fetching packages...
warning [email protected]: The platform "win32" is incompatible with this module.
info "[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
[4/4] Building fresh packages...
success Saved lockfile.
warning Your current version of Yarn is out of date. The latest version is "1.0.2" while you're on "0.27.5".
$ node ./gulp/post-install.js
Reload client successfully installed.
Done in 43.23s.

@ghost
Copy link
Author

ghost commented Sep 14, 2017

Based on your explanation, it actually seems to be relying on tslint's existence on the top level whcih indicates that it relies on it.

Our build-tool package is intended to package up an entire build and expose, not tools, but workflows, to the end application.

So, build-tool provides "linting" to the application, but does not provide insight into the specific implementation (other than the "linting" workflow supports TypeScript).

Therefore we wouldn't expect the application to install tslint. tslint is an implementation detail of build-tool.

@ghost
Copy link
Author

ghost commented Sep 14, 2017

That said if you think about it, as long as build-tool can access tslint and when other packages also list tslint as a peerDependency, they all use the same one, this outcome is "correct".

It's gulp-tslint that's requireing tslint, and because tslint is under build-tool while gulp-tslint has been hoisted one directory higher, require fails.

Is it acceptable for packages like gulp-tslint to be calling require on a package listed as a peerDependency?

@BYK
Copy link
Member

BYK commented Sep 14, 2017

@destroyerofbuilds okay, thanks a lot for the detailed explanation. I've looked at all package.json files and it looks like there's some incorrect hoisting happening somewhere. gulp-tslint correctly defines tslint as a peer dependency and the consumer of gulp-tslint also defines tslint as a dependency on the side.

In this case, the expected behavior is for gulp-tslint to be able to require tslint which is not the case here since it is hoisted one level higher than tslint.

Please keep your repo that shows this around until we resolve this bug. Thanks again for your time and help!

@BYK BYK self-assigned this Sep 15, 2017
BYK added a commit that referenced this issue Sep 15, 2017
**Summary**

Fixes #4446, fixes #4433. Follow up to #3893. The fix in #3893 was
too aggressive, allowing only top-level dependencies to be used in
peer dependency resolution which was incorrect. This patch allows
resolving peer dependencies from the same or higher levels in the
dependency tree.

**Test plan**

Additional unit and integration tests.
@BYK BYK closed this as completed in #4478 Sep 16, 2017
BYK added a commit that referenced this issue Sep 16, 2017
…4478)

**Summary**

Fixes #4446, fixes #4433, fixes #2688, fixes #2387. Follow up to #3803. The fix in #3893 was
too aggressive, allowing only top-level dependencies to be used in
peer dependency resolution which was incorrect. This patch allows
resolving peer dependencies from the same or higher levels in the
dependency tree.

**Test plan**

Additional unit and integration tests.
joaolucasl pushed a commit to joaolucasl/yarn that referenced this issue Oct 27, 2017
…arnpkg#4478)

**Summary**

Fixes yarnpkg#4446, fixes yarnpkg#4433, fixes yarnpkg#2688, fixes yarnpkg#2387. Follow up to yarnpkg#3803. The fix in yarnpkg#3893 was
too aggressive, allowing only top-level dependencies to be used in
peer dependency resolution which was incorrect. This patch allows
resolving peer dependencies from the same or higher levels in the
dependency tree.

**Test plan**

Additional unit and integration tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants