Anything imported by generated code should be a peerDependency

[andreas-karlsson]

Even if dependencies of ts-proto usually can be transitively reached from generated code, it's not in strict adherence with the node resolution algorithm.

For example the above assumption can be broken when there are version conflicts and npm/yarn decides to install a dependency local to ts-proto. It's also broken by other package managers that don't use a flat/hoisted node_modules structure like pnpm.

This can be fixed by declaring every package imported by generated code, as a peer dependency to ts-proto.

[stephenh]

Yeah, I guess that makes sense. I'm a little concerned that this might be a breaking change, or at least mean downstream users get peer dependency warnings, but we can try it and see how it goes.

A wrinkle is that, as mentioned in the commit message, ts-proto itself probably shouldn't have any dependencies for the generated code at all, b/c its very conditional (based on the flags users pass to ts-proto) which packages will/will not be needed at runtime.

I.e. if you're not using the dataloader option, it doesn't make sense for that to be a dependency. Or if you're using the only types option, you don't really need/want any dependencies at all, other than just ts-proto itself as a devDependency for the code generator and that's it.

I'll defer this wrinkle to another issue / until someone complains about it / patches it. :-)

[stephenh]

Ah yeah, I should have done a pull request, but this change even broken ts-proto's test suite, b/c the peer dependencies, specifically protobufjs, weren't installed automatically.

So I've reverted this change and will re-consider when ts-proto is doing a major release when we can sneak it in as a breaking change. I also filed #159 to think about the setup in general.

[D4nte]

I am guessing this is why it is not possible to use yarn 2 with ts-proto? I am getting the following error:

/home/froyer/src/status-im/js-waku/.pnp.js:17184
    throw firstError;
    ^

Error: ts-proto-descriptors tried to access long, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.

Required package: long (via "long")
Required by: ts-proto-descriptors@npm:1.2.0 (via /home/froyer/src/status-im/js-waku/.yarn/cache/ts-proto-descriptors-npm-1.2.0-46b3084b16-7a61cb5172.zip/node_modules/ts-proto-descriptors/google/protobuf/compiler/)

Require stack:
- /home/froyer/src/status-im/js-waku/.yarn/cache/ts-proto-descriptors-npm-1.2.0-46b3084b16-7a61cb5172.zip/node_modules/ts-proto-descriptors/google/protobuf/compiler/plugin.js
- /home/froyer/src/status-im/js-waku/.yarn/cache/ts-proto-npm-1.79.2-abea3dda18-2d1d05fd26.zip/node_modules/ts-proto/build/plugin.js
- /home/froyer/src/status-im/js-waku/.yarn/cache/ts-proto-npm-1.79.2-abea3dda18-2d1d05fd26.zip/node_modules/ts-proto/protoc-gen-ts_proto
    at internalTools_makeError (/home/froyer/src/status-im/js-waku/.pnp.js:16928:34)
    at resolveToUnqualified (/home/froyer/src/status-im/js-waku/.pnp.js:17893:23)
    at resolveRequest (/home/froyer/src/status-im/js-waku/.pnp.js:17985:29)
    at Object.resolveRequest (/home/froyer/src/status-im/js-waku/.pnp.js:18063:26)
    at Function.external_module_.Module._resolveFilename (/home/froyer/src/status-im/js-waku/.pnp.js:17161:34)
    at Function.external_module_.Module._load (/home/froyer/src/status-im/js-waku/.pnp.js:17026:48)
    at Module.require (node:internal/modules/cjs/loader:996:19)
    at require (node:internal/modules/cjs/helpers:92:18)
    at Object.<anonymous> (/home/froyer/src/status-im/js-waku/.yarn/cache/ts-proto-descriptors-npm-1.2.0-46b3084b16-7a61cb5172.zip/node_modules/ts-proto-descriptors/google/protobuf/compiler/plugin.js:16:12)
    at Module._compile (node:internal/modules/cjs/loader:1092:14)
/home/froyer/src/status-im/js-waku/.pnp.js:17184
    throw firstError;
    ^

[stephenh]

@D4nte I believe the ts-proto-descriptors is slightly different, i.e. afaiu it should have a direct dependency on long, which you're right it's missing; I believe I've fixed in #275 , which will show up as a new release soon.

[psalz]

I just ran into this with Yarn PnP which does not resolve packages that aren't explicitly listed as dependencies, i.e., I had to manually install both long and protobufjs. If these are not added as peerDependencies, maybe it should at least be documented somewhere.

[stephenh]

@psalz isn't protobufjs already a dependency of ts-proto?

...fwiw I think personally in downstream projects, I would mark ts-proto as a devDependency (it's a code generator that is only used at build time) and then add explicit dependencies to protobufjs and long anyway.

Or @psalz is this what you're saying, in that to get Yarn PnP to like the generated code, your own project needed explicit dependencies on the imports that were in the generated code?

If so, that makes sense. If you want to add a note in the readme somewhere that makes sense, that'd be great.

[D4nte]

...fwiw I think personally in downstream projects, I would mark ts-proto as a devDependency (it's a code generator that is only used at build time) and then add explicit dependencies to protobufjs and long anyway.

Yes this is what I have done, maybe worth mentioning it in the README?

[psalz]

Or @psalz is this what you're saying, in that to get Yarn PnP to like the generated code, your own project needed explicit dependencies on the imports that were in the generated code?

Exactly. Even though protobufjs is transitively included through ts-proto, Yarn PnP won't allow you to require it unless it's specified as a top-level dependency.

I would mark ts-proto as a devDependency (it's a code generator that is only used at build time) and then add explicit dependencies to protobufjs and long anyway.

You're right, that is probably the best way of doing it. Although adding the peer-dependencioes to ts-proto might still not be a bad idea: I don't know how npm handles this, but from what I can tell yarn will not install peer-dependencies automatically, instead issuing a warning in case something is missing. This way there would be no added bloat from the long package (protobufjs is transitively required anyway), but the user would still be informed about it.

If you want to add a note in the readme somewhere that makes sense, that'd be great.

I can, although I'm not very clear on the specifics: protobufjs is always required right? And long only for certain configurations? Is there any other packages that might be required?