-
Notifications
You must be signed in to change notification settings - Fork 415
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
fix: 🗑️ Deprecation notice for js-ipfs #1628
Conversation
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
Per ipfs/ipfs-companion#1220 (comment), I would encourage removing this all-together. I assume we aren't testing this functionality and it's not supported. As a result, I don't see value in advertising it. |
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, but I would add a tracking issue on the companion side for users to follow so they will know when it's done.
Closing that issue would also be a good indicator that these docs need updating again.
Also, +1 to what Steve said.
@SgtPooki the checks seem to be failing because of an error link in a file that didn't change. merge with errors? 🤔 |
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 with a slight tweak
@whizzzkid are there any other changes you'd like to make before I merge this into https://github.com/ipfs/ipfs-docs/tree/js-ipfs-cleanup for final cleanup and merging? |
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.
I put a couple of comments but ultimately created a new PR here #1631 @whizzzkid
::: warning DEPRECATED | ||
|
||
js-ipfs has been deprecated in favor of [Helia](https://github.com/ipfs/helia) and will be removed in a future release of IPFS Companion. Improvements to Helia are ongoing, and we hope to have it ready for use in IPFS Companion once critical features like [webRTC Streams](https://github.com/w3c/webextensions/issues/72) are available in Chrome Manifest V3. | ||
|
||
::: |
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.
@whizzzkid : I think this just adds noise. This can be in our backlogged but this was a non-functioning feature. I think we should just strip it out.
@@ -67,6 +67,12 @@ This node type offers the same benefits as an [external](#external) node, with a | |||
|
|||
## Embedded |
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.
Can we just remove the full Embedded section?
Describe your changes
Files changed
What issue(s) does this address?
Checklist before requesting a review
Checklist before merging