Support for multiple dependencies #17
Conversation
| return info.package.getVersion(info.version); | ||
| } | ||
|
|
||
| function getPackage(string packageName) public view returns (Package, string) { |
There was a problem hiding this comment.
Given that the mapping is internal, we have no other way to access a package info
|
|
||
| function unsetPackage(string packageName) public onlyOwner { | ||
| require(address(providers[packageName].package) != address(0), "Package to unset not found"); | ||
| delete providers[packageName]; |
There was a problem hiding this comment.
Missing PackageChanged event here (and a test for it?).
| } | ||
|
|
||
| async install() { | ||
| await npm.install([this.nameAndVersion], { save: true, cwd: process.cwd() }) |
There was a problem hiding this comment.
There is no need of using async await here, as npm.install(...) is returning a promise that is being awaited in the function call
There was a problem hiding this comment.
Agree, but I have actually started using async/await in these cases to make it more explicit that the function should be awaited. If a user of this class sees an install() function, they may be inclined to call it directly; if they see async install(), they will certainly await it.
That said, when we migrate to typescript, as we annotate the function to return a promise, we can remove this :-P
| } | ||
| } | ||
|
|
||
| static satisfiesVersion(version, requirement) { |
There was a problem hiding this comment.
I would move all static methods to the top of the class
| const app = this.project.getApp() | ||
| this.networkFile.app = { address: app.address }; | ||
|
|
||
| const thepackage = await this.project.getProjectPackage() |
There was a problem hiding this comment.
use lowerCamelCase and maybe rename this variable to projectPackage or something more descriptable?
There was a problem hiding this comment.
Good idea. thepackage is the only case where I use all-lower, because strict mode prohibits using package as a variable name.
packages/cli/src/scripts/update.js
Outdated
| const proxies = await controller.upgradeProxies(contractAlias, proxyAddress, initMethod, initArgs); | ||
| _(proxies).values().forEach(proxies => proxies.forEach(proxy => stdout(proxy.address))); | ||
| const proxies = await controller.upgradeProxies(packageName, contractAlias, proxyAddress, initMethod, initArgs); | ||
| _.forEach(proxies, proxy => stdout(proxy.address)); |
There was a problem hiding this comment.
Suggestion: You could use proxies.forEach(proxy => stdout(proxy.address)) instead, and avoid lodash lib import (as this is the only lodash function call that is being used in this file). In addition to this, we could start importing only necessary lodash functions and not the whole lib (i.e., import { forEach } from 'lodash' or import { forEach as _forEach } from 'lodash' )
| _.map(promisesWithObjects, handlingFailure) | ||
| ) | ||
|
|
||
| if(!_.isEmpty(failures)) { |
There was a problem hiding this comment.
You can move this conditional above the result variable assignment and avoid unnecessary promise management
App contract now has multiple providers, instead of having a single one which could have a fallback (stdlib). Now the app explicitly manages its dependencies. App is still offered in two flavours: unversioned (which accepts directories) and versioned (which accepts packages with a pinned version). All contract wrapper models were also updated. In particular, App no longer acts as a facade, but is instead an actual wrapper of the App contract. A new abstraction, Project, is introduced to be used as a facade by the CLI (or lib users). Project is subclassed by LibProject and AppProject.
spalladino
force-pushed
the
feature/multiple-deps
branch
from
1d076fe
to
c3657de
Compare
spalladino
removed
the
status:in-progress
Tasks
Lib
App contract now has multiple providers, instead of having a single
one which could have a fallback (stdlib). Now the app explicitly manages
its dependencies. App is still offered in two flavours: unversioned
(which accepts directories) and versioned (which accepts packages with a
pinned version).
All contract wrapper models were also updated. In particular, App no
longer acts as a facade, but is instead an actual wrapper of the App
contract. A new abstraction, Project, is introduced to be used as a
facade by the CLI (or lib users). Project is subclassed by LibProject
and AppProject.
Part of #41