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

feat: Provide access to bundled libraries when in browser #1297

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Apr 4, 2018

In this PR, the bundled libraries mentioned in 406, as well as ipfs/js-ipfs#525 (comment) were added the same way as in js-ipfs-api.

For this issue, other 2 PRs were created

@vasco-santos vasco-santos force-pushed the feat_provide-access-to-bundled-libraries-when-in-browser branch from d884f05 to 7861794 Compare April 5, 2018 11:02
daviddias
daviddias previously approved these changes Apr 14, 2018
@alanshaw
Copy link
Member

Just wondering why these types are properties of an instance and not of the constructor/separate export?

@vasco-santos
Copy link
Member Author

Hi @alanshaw

At first, I was thinking on a separate export, as you suggest. It is described here why the solution was the implemented.

@alanshaw
Copy link
Member

hmm, I'm still not sure I understand the reasoning.

The util on js-ipfs-api instances contained functions that operate on the instance you've created - that makes sense. Putting non-instance related types/functions in an instance is a little odd. i.e. getting hold of CID: new IPFS().types.CID as opposed to const { CID } = IPFS.types.

Needless to say, if someone wanted to do something with CID before creating their IPFS node then they can't.

@vasco-santos
Copy link
Member Author

I agree that it seems odd @alanshaw . However, if the js-ipfs is imported, I don't understand the point of not creating a node at first.

Anyway, I agree that it is a strange behavior and it should be improved. What do you suggest to support this, maintaining the coherency across repos?

@daviddias
Copy link
Member

Let's get a decision on this asap to get both interfaces consistent. I'm fine with having these as static references (meaning, available through class and instance)

@daviddias daviddias mentioned this pull request Apr 30, 2018
30 tasks
@daviddias
Copy link
Member

daviddias commented Apr 30, 2018

I agree with @alanshaw. What about having both? @alanshaw wanna submit a separate PR to add the types as static exports too?

Meanwhile I'm coalescing this PR with #1319 so that we can finally get CI green again and work from a clean table.

@daviddias daviddias changed the base branch from master to fix/files.add-pull-streams April 30, 2018 20:06
@daviddias daviddias merged commit 180db8c into fix/files.add-pull-streams Apr 30, 2018
@daviddias daviddias deleted the feat_provide-access-to-bundled-libraries-when-in-browser branch April 30, 2018 20:07
@ghost ghost removed the status/in-progress In progress label Apr 30, 2018
@mitra42
Copy link

mitra42 commented Dec 4, 2018

Did something stall this, I agree with @alanshaw that having this on static is significantly more useful, as we have code that doesn't easily have access to the ipfs node that makes this check by reimporting the "cids"

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.

4 participants