Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

chore: remove ipld-dag-cbor dependency #1618

Closed
wants to merge 1 commit into from
Closed

chore: remove ipld-dag-cbor dependency #1618

wants to merge 1 commit into from

Conversation

vmx
Copy link
Member

@vmx vmx commented Oct 4, 2018

The ipld-dag-cbor module isn't used anywhere, hence remove it
as a direct dependency.

@ghost ghost assigned vmx Oct 4, 2018
@ghost ghost added the status/in-progress In progress label Oct 4, 2018
The `ipld-dag-cbor` module isn't used anywhere, hence remove it
as a direct dependency.
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, that reference was added in order to allow an easy access to it from the browser instance. As it is a dependency of a dependency (ipld in this case), there is no extra space in the final js-ipfs bundle to allow an easy access to it.

This way, I think we may keep it that way. What do you think @vmx @diasdavid @alanshaw ?

@vmx
Copy link
Member Author

vmx commented Oct 4, 2018

@vasco-santos I can see the point for other modules. But I don't think you should ever need direct access to ipld-dag-cbor. And if you do, just bundle it yourself. The problem I see here is that if we give access here, it might even be a different version from what js-ipld is using. Hence I think it would cause more pain than benefits.

@vasco-santos
Copy link
Member

I see. And what about dagPB? Should we keep it?

@vmx
Copy link
Member Author

vmx commented Oct 4, 2018

I would say IPLD formats are implementation detail, so I'd also be for removing ipld-dag-pb if everyone agrees.

@vmx
Copy link
Member Author

vmx commented Oct 9, 2018

@vmx vmx closed this Oct 9, 2018
@ghost ghost removed the status/in-progress In progress label Oct 9, 2018
@achingbrain achingbrain deleted the remove-dag-cbor branch December 3, 2020 16:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants