-
Notifications
You must be signed in to change notification settings - Fork 493
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
chore: migrate ipfs-api to ipfs-http-client #1647
Conversation
DeepCode analyzed this pull request. Click to see more details. |
48b0d50
to
e07dc09
Compare
@@ -2,10 +2,15 @@ import { __ } from 'embark-i18n'; | |||
import { dappPath, embarkPath } from 'embark-utils'; | |||
var Npm = require('./npm.js'); | |||
|
|||
const DEPRECATIONS = { | |||
'ipfs-api': 'ipfs-http-client' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! As part of the deprecation logic do we need to pair this with a default version of ipfs-http-client
to use in case ipfs-api
is found in embark.json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question!
This will use the default version for the replacement, so it'll forward the call to version:get:ipfs-http-client
. That version comes from this.versions
:
This would then be registered and used here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for thinking about the upgrade path. Should we maybe add a link in the deprecation warning to a blog post or some documentation or maybe even just inline instructions on further action for the user? For example, "Please update embark.json versions to use ipfs-http-client instead of ipfs-api".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too. I like @emizzle 's suggestion though
@andremedeiros since we need to hold off on this package upgrade until embark |
Deprecates #1535