why is popsicle in the devDependencies only?

[zkochan]

Hi, I am testing how pnpm works on some of the most popular Node packages. I was doing a pnpm install on the rxjs repo and I noticed that the prepublish script failed because popsicle-retry could not find popsicle.

I understand that this works with npm@3 and yarn, as they create a flat dependency tree, but is there a reason not to include popsicle as a dependency? You can at least include it as a peerDependency, I think.
It doesn't make sense to have it in devDependencies. You either need it in dependencies or in dev deps + peer deps, am I right?

[blakeembrey]

No, not right. I don't have to specify it as a peer dependency for it to work. It's also not a dependency, it's used with popsicle and does not use popsicle nor the other way around. Sounds like a bug in pnpm, it shouldn't be running prepublish nor would prepublish execute successfully from npm (it relies on all devDependencies).

[zkochan]

No, not right. I don't have to specify it as a peer dependency for it to work. It's also not a dependency, it's used with popsicle and does not use popsicle nor the other way around.

In that case, why do you import the Request class from popsycle at L3 and create an instance of it at L27?

            return resolve(new Request(options))

it should never be running prepublish nor would prepublish ever execute successfully from npm (it relies on all devDependencies).

Prepublish in your repo works fine with pnpm. I was talking about prepublish of rxjs

[blakeembrey]

In that case, why do you import the Request class from popsycle at L3 and create an instance of it at L27?

Ok, so it does use it in the latest version (I thought it was only in type positions), but that shouldn't change anything. There's no need to rely on a flat structure or nested structure, this module works fine with all NPM versions. I think there's an issue in pnpm, in that it's not setting up the node_modules correctly. Node module resolution would allow you to automatically resolve dependencies recursively up the tree when it's not found, and since this module can only ever be used with popsicle, I don't see the issue (it'll look in the current node_modules and then up again and find it).

[zkochan]

pnpm uses symlinks to link packages from the store to node_modules. Symlinks are resolved to realpath by Node. If you would at least add popsicle as a peer dependency, it would work with pnpm, because pnpm links peer deps to node_modules. IMHO, if you use a package, it should be specified in package.json at least as a peer. Unless it is a package like eslint that searches for its plugins, but that is another use case

[blakeembrey]

I don't see any reason it needs to be specified, it relies on extremely common and documented behaviour of node's module system. It's a peer dependency and NPM has trouble with pre-releases in peer dependencies positions so I typically avoid it. If you really wanted, you can set it in package.json, but blaming it on this package seems like the wrong approach - you'll just run into new issues with other packages later. I don't think this is a very uncommon use-case. Also, what version of node are you using? I believe the realpath behaviour was changed in v6 (nodejs/node#5950).

[zkochan]

That change was reverted in Node v6.3.

Don't get me wrong, I am investigating. If you think this is fine you might be right, I will open an issue on pnpm and discuss it there.

[blakeembrey]

Ah, that's unfortunate. I thought it was a worthwhile change. I'm happy to merge a PR to add it as a wildcard peer dependencies, I'm not sure if that'll mess with anything, but it's probably fine. I still believe, in the long run, figuring out how to workaround this might be worthwhile. I'm sure there's more modules than just mine that have relied on how NPM and node modules resolve (which happens to work for both flat and nested trees here, just not with symlinks).

[zkochan]

Thanks! I will try to find out whether this is a widely-used pattern and how to best solve it before creating a PR.

Regarding Node, that is unfortunate, for sure, at least from pnpm's perspective :-) But I think I did manage to come up with a structure that solves most of the issues in pnpm 0.47.

Also there is a discussion about trying to fix the symlinking issue in Node: nodejs/node-eps#46

[blakeembrey]

Thanks for the link, it seems using symlinks would actually break npm link sub-dependencies, so it makes sense on reverting it. I'm not sure how to fix this in the pnpm case, but a PR is definitely welcome to improve it. Ideally NPM would make peer dependencies less painful for me so that I didn't avoid this to begin with 😛

[iamstarkov]

I don't see any reason it needs to be specified, it relies on extremely common and documented behaviour of node's module system. It's a peer dependency and NPM has trouble with pre-releases in peer dependencies positions so I typically avoid it.

reason to be specified: its production used dependency. It might be true that everything seems to working correctly at the time im typing this. But (a) its broken contract of shared dependency environment (everything you need is specified in pkg) (b) there is no guarantee it wouldnt be broken in the future

[iamstarkov]

and let say is it working with npm@2?

[blakeembrey]

If you looked at it, it works with NPM 2, 3, 4, whatever version you want - you can check CI. There is a guarantee it wouldn't be broken, because that's how the node module system works. I think you're misunderstanding something - this module can only ever be used with popsicle, which means the user has installed popsicle already. Because of how node modules resolve, it'll always work.

[blakeembrey]

Also, I'm pretty confident you'll have issues with https://github.com/TypeStrong/ts-loader and https://github.com/TypeStrong/ts-node - those are off the top of my head because I work with them often. They also use typescript in the same manner, not as a direct dependency but used when the user installs them because of how module resolution works.

[iamstarkov]

thanks for explanation