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

RFC: Install user-defined dependencies centrally, instead of letting adapters handle them #2006

Closed
AlCalzone opened this issue Sep 20, 2022 · 9 comments · Fixed by #2650
Closed

Comments

@AlCalzone
Copy link
Collaborator

AlCalzone commented Sep 20, 2022

"Manually" installing dependencies in a sub-package of our "install" project (/opt/iobroker/package.json) once worked fine, but increasingly collides with what npm does while managing the dependencies. We already stopped doing a separate npm install inside the adapter directories in #1339, but some adapters still do this, leading to weird issues.

I therefore propose we get rid of this too and do the installation in a central location, adding the dependencies to our root package.json. To avoid conflicts, we need to fake-namespace these packages, which npm supports using the npm: protocol. Npm supports this since 6.9.0 (npm/cli#3):

// package.json
"dependencies": {
  //... all adapters etc., as usual
  // namespaced user-defined dependencies of iobroker.javascript, instance 0:
  "@iobroker-javascript.0/axios": "npm:axios@1.2.3", // Here javascript.0 has axios@1.2.3 as its dependency
  "@iobroker-javascript.0/alcalzone-pak": "npm:@alcalzone/pak@0.9.0", // scoped dependencies need to be escaped. I propose omitting the leading @ and replacing / with - (_ would also be fine)
  // ... likewise for other adapters/instances
}

This has the downside that the adapters need to be able to redirect the require to the correct package name, e.g. javascript.0 would have to require @iobroker-javascript.0/axios instead of just axios, but we can expose a utility for this too.

The upside would be that versioning these dependencies would become a bit more manageable, and another adapter installing the same one would not accidentally cause a conflict. This way we can also support multiple instances of javascript have different versions of the same package, since the fake scopes are instance-specific. In addition, maybe the adapters wouldn't even have to store their dependencies separately anymore, but could rather enumerate them from the root package.json.

@Apollon77
Copy link
Collaborator

I'm not sure how this would work for node-red ... here we have no control over the application itself and we only start it as extra process

@Apollon77
Copy link
Collaborator

see ioBroker/ioBroker#512 : When we change this we also need to prevent concurrent runs.

Other question: can we not define an own "npm root folder "for each adapzter and see keep these adapter relevant npm trees separated and make (somehow ;-) ) sure that the relevant local stuff uses the special ones instead of the omnes fropm adapte rby somehow manipulating resolve logic?

@foxriver76
Copy link
Collaborator

see ioBroker/ioBroker#512 : When we change this we also need to prevent concurrent runs.

Other question: can we not define an own "npm root folder "for each adapzter and see keep these adapter relevant npm trees separated and make (somehow ;-) ) sure that the relevant local stuff uses the special ones instead of the omnes fropm adapte rby somehow manipulating resolve logic?

So basically the proposal from Dominic but each adapter has a separate folder? What should the advantage be compared to the proposed solution? Reducing risk of conflicts? Still with two js instances, they have the same folder or you mean one "root" folder per instance?

@Apollon77
Copy link
Collaborator

I would mean one per instance to really separate them ... in theory one JS adapter could use library@1.x and a second instance library@2.x ... what to access? How that is handled correctly? ;-) I know edge cases, but if we now plan to restructure it then why not in such a way - amd I think node-rted also actually does exactly this

@foxriver76
Copy link
Collaborator

indeed sounds reasonable, need to play around a bit when time has come. For sure want to have this in next controller

@AlCalzone
Copy link
Collaborator Author

AlCalzone commented Mar 21, 2024

npm root folder "for each adapzter

Soo, use npm workspaces and have each adapter installation be a workspace?

@foxriver76
Copy link
Collaborator

foxriver76 commented Mar 25, 2024

I think what @Apollon77 wanted to achieve is separated packages for each adapter to manage the dependencies of this instance, so no global workspace root which would then conflict again, but depending on what npm does internally this is likely not that optimal.

So as I understood e.g. like /opt/iobroker/node_modules/iobroker.javascript.0.dependency-mgmt then place package json there with the packages installed by javascript instance 0 and so on. Also with the approach like from initial post

// package.json
"dependencies": {
  //... all adapters etc., as usual
  // namespaced user-defined dependencies of iobroker.javascript, instance 0:
  "@iobroker-javascript.0/axios": "npm:axios@1.2.3", // Here javascript.0 has axios@1.2.3 as its dependency
  "@iobroker-javascript.0/alcalzone-pak": "npm:@alcalzone/pak@0.9.0", // scoped dependencies need to be escaped. I propose omitting the leading @ and replacing / with - (_ would also be fine)
  // ... likewise for other adapters/instances
}

@AlCalzone
Copy link
Collaborator Author

AlCalzone commented Mar 25, 2024

Putting custom folders inside node-modules is just asking for trouble. Besides I don't know how we would even tell npm to install there.

If we changed the installation method so that the root directory was a workspace root and each adapter a workspace, then npm should automatically install the dependencies in the correct location.
But this would mean we can't just npm install iobroker.adaptername in the root, which is probably too breaking.

I think the idea from the OP is the most compatible one, even if it's a bit weird.

@foxriver76
Copy link
Collaborator

Yes I am with you initial approach is best.

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.

3 participants