POC: Use peer dependencies for @theia dependencies #5939
Conversation
|
I may have mixed two different problems here: peer deps and yarn audit. Will unclutter once back on this. |
@marechal-p it is not enough, we should verify that it actually fix something before merging for end clients i.e. we need to create a standalone app which consumes these packages but with different version ranges and see what happens. I think you will need to make your fork, publish them under another scope and create a test app. |
Better do it separate. |
|
@marechal-p before continuing trying to integrate it, please make an example of whether it is really solving anything with some simple packages, it does not even need to be theia extension packages. Generally dependency issues won't be a big issue for extension developers, only for IDE developers. Extension developers should use the plugin system which does not have such drawbacks. IDE developers are in the control of building the app, so they can easily workaround dependency conflicts. I would like to avoid complicating the setup if it replaces some drawbacks with others. I.e. declaring all transitive dependencies is for sure breaking for IDE developers, and for us we need new scripts to keep dependencies in sync. |
paul-marechal
force-pushed
the
mp/peer-dependencies
branch
from
81e04f6
to
7de94ec
Compare
Only The biggest advantage I see is that using peer+dev deps makes this project rely on good practices, rather than using a simple but broken method (making everything prod deps) and then trying to fix it after. That if peer deps really solve the "host package" issue. Things should break more gracefully using peer deps, but that's still to be tested indeed. |
|
I'm not sure what you are referencing as best practices. If a packages is using apis of another package when it should have a runtime dependency. Peer dependencies should be used when a package is not using apis but should be installed as well. The first is true for all theia extensions packages, the second is false for all them. This PR breaks it forcing theia packages declare dependencies in no natural way, i.e. (1) we redeclare them as dev dependencies because they are not runtime anymore, when they should be, which is obviously misusage of peer dependencies; (2) we force IDE developers to redeclare all theia transitive dependencies which also misusage. What is broken? npm and yarn allow duplicate packages and it is a default in js ecosystem. I doubt that they behave differently for peer dependencies, since it is not even considered to be an issue for them. I have not found that this issue will go away from links which you referenced. Please back up your claims first. Here is an issue for yarn for example to support it: yarnpkg/yarn#422 |
Peer dependencies are meant to be somewhat unique, in our case totally unique, since yarn/npm doesn't automatically pull peer deps like it would for standard prod/dev deps.
I'm sorry but I think this is not true. See https://github.com/domenic/chai-as-promised/blob/master/package.json#L31-L35 https://github.com/domenic/chai-as-promised/blob/master/lib/chai-as-promised.js#L5-L8 It both depends on This is all according to https://nodejs.org/es/blog/npm/peer-dependencies/. Peer dependencies express the relationship between extensions/plugins and their "host" package meant to run with them.
Peer dependencies express runtime dependencies. And what I mean by best practices is not using a fork to cut bread. While we can make it work, I wanted to see how other solutions such as peer dependencies would help. I was in the process of writing a small demo setup to see them in action, I might finish it later once I figure out implementation details. This draft PR is meant to try it first hand on the repo, and I think that given the correct configuration, it can work better this way.
Peer dependencies solve the "singleton" issue we have with extensions: there shouldn't be Re-declaring the peer deps for a specific package as a dev dep might be a bit historic as well as a lerna limitation? Basically, since peer deps are not automatically pulled, when you are developing you may want to run tests that would execute code in your package. But if your peer dep is missing, then it won't run. This is only an issue at development time, hence why the dependency is listed as a dev dependency. In an application depending on a package with a peer dependency, it is onto the application maker to also depend on the correct package required as a peer dependency. Usually the way things go is that you are already depending on In the Theia monorepository, since we are using yarn workspaces, all of our packages are all hoisted up in the main node_modules folder (note that hoisting is only an optimization and that we should try our best not to rely on this). This means that by declaring peer deps without declaring them as dev deps, packages can still reference each others at runtime. The issue happens when building: since lerna doesn't look at peer dependency to compute the topological order of our setup, it tries to build everything out of order, resulting in missing references (core did not build before extensions depending on it). To be fair, since we only have to declare extensions on which a package depends as a dev dep, we can write a rather flexible version range, something like
To be extra clear, I am only talking about Theia extension packages, not any other package. And either we can automate it, or yarn will tell them at build time that peer dependencies are missing. I will keep setting up an example setup using peer dependencies and report back, I will close this draft/poc if I ever start thinking that this is not the way forward. |
When building a Theia application, we do not want multiple extension versions to be downloaded under each dependencies that requires it. [1] Since lerna doesn't look at peer dependencies for topological ordering, we need to also register peer dependencies as dev dependencies. [2] [1] https://nodejs.org/es/blog/npm/peer-dependencies/ [2] lerna/lerna#1892 (comment) Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
paul-marechal
force-pushed
the
mp/peer-dependencies
branch
from
7de94ec
to
490bc11
Compare
|
https://github.com/marechal-p/test-peer-dependencies This repository allows one to test peer dependencies. Issue is that we have to meddle with some local npm registry in order to publish packages without polluting the main npm registry. After some tests, when packages use peer dependencies, yarn won't automatically pull a package. It is on the user to find one that matches everyone's range. If the package picked does not match everything, a warning is emitted. If the peer dependency is not included in the application, a warning will be emitted as well. |
Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
|
Applications now correctly start. |
|
@svenefftinge can you review it please |
|
@svenefftinge it will affect how all IDE and extension developers consume Theia extensions, plus how they should define their dependencies. I'm not sure that we want such rippling effect and a change worth of it, taking into account that we have other priorities right now. It would be good to get your input. |
What it does
It moves all
@theiadependencies to "peer dependencies". This should allow the dependency tree to only look for one extension version instead of duplicating it. See https://nodejs.org/es/blog/npm/peer-dependencies/Only quirk is that in order for lerna to understand the package topology, we need to declare the packages as dev dependencies.
How to test
Build and run (might be broken for now, since this is a draft/poc)
Review checklist
Reminder for reviewers