feat: add filecoin to dynamic CLI #726
Conversation
mikeseese
force-pushed
the
filecoin
branch
2 times, most recently
from
b2549c8
to
67cde24
Compare
mikeseese
force-pushed
the
feat/add-filecoin-to-dynamic-cli
branch
from
db1c5e8
to
482c72a
Compare
mikeseese
force-pushed
the
feat/add-filecoin-to-dynamic-cli
branch
from
acec01f
to
50aa0aa
Compare
| @@ -55,6 +55,7 @@ | |||
| "@ganache/colors": "^0.1.0", | |||
| "@ganache/ethereum": "0.1.0", | |||
| "@ganache/ethereum-options": "0.1.0", | |||
| "@ganache/filecoin": "0.1.0", | |||
There was a problem hiding this comment.
Since we don't want filecoin shipping with cli (and ganache) I think this should be a peerDependency, and the version should probably be "^0.1.0". This gets into the "fancy downloader" convo, so maybe it's time we have that.
Perhaps we should add the download into @ganache/core's connector-loader.ts? I figure @ganache/core would also need to detect that a flavor is valid but not installed; of course, when used programmatically it wouldn't provide an interactive installation, but it could still perform the same error logic.
Now that I've typed all that... for now we could just show a helpful error message if the flavor dependency isn't already installed. e.g., 'The "filecoin" flavor of ganache is not installed. Install it by running npm install @ganache/filecoin' (obviously it's a little more complex if we want to determine global installation, package manager used, etc)
There was a problem hiding this comment.
This gets into the "fancy downloader" convo, so maybe it's time we have that.
Because of this, I'm favoring that we leave this as is until we're actually considering merging this into develop. That way I can continue working without any fancy stuff. We can determine the delivery mechanism and how I need to support that later. I don't think we should work any of this part until we're closer to merging this into develop (probably early March)
and the version should probably be
"^0.1.0"
When should versions be pinned and when shouldn't they be? This just did whatever the default lerna add behavior was, but I am trying to pin everything new I add due to the contribution guidelines
There was a problem hiding this comment.
FYI I went ahead and added initial support for this as it's necessary for some of my future PRs to be acceptable for you (otherwise I'd have to turn off all tests with node versions less than 12 rather than just filecoin tests)
I added the logic in @ganache/flavors. I initially was thinking of putting it in connector-loader.ts like you, but something felt better about putting it in flavors. We can definitely discuss more whenever we review that work
| @@ -43,6 +43,8 @@ | |||
| "dependencies": { | |||
| "@ganache/ethereum": "^0.1.0", | |||
| "@ganache/ethereum-options": "0.1.0", | |||
| "@ganache/filecoin": "0.1.0", | |||
There was a problem hiding this comment.
This should also probably be a miner-semver peerDependency here as well (tezos technically needs to be moved as well)
There was a problem hiding this comment.
You can move tezos on develop if you'd like. My response from #726 (comment) applies here too
Co-authored-by: David Murdoch <[email protected]>
…oin-to-dynamic-cli' of github.com:trufflesuite/ganache-core into feat/add-filecoin-to-dynamic-cli
|
We're postponing these reviews until later |
This PR is the filecoin-specific changes to support the CLI. This PR is part of #689