Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

Provide access to bundled libraries when run in browser #406

Closed
lidel opened this issue Oct 31, 2016 · 10 comments · Fixed by #732
Closed

Provide access to bundled libraries when run in browser #406

lidel opened this issue Oct 31, 2016 · 10 comments · Fixed by #732

Comments

@lidel
Copy link
Contributor

lidel commented Oct 31, 2016

I wonder if there are any cons to exposing libraries such as is-ipfs, multiaddr and multihashes under ipfs.util.lib.* or maybe only in browser-specific build under windows.ipfsLibs ?

I feel people who use browserified artifact would really appreciate it as it would remove code duplication in smaller projects (no need to ship library twice just to have it available as window.foo).

Perhaps there are better ways to do this that I am not aware of? Any thoughts on this?

@daviddias
Copy link
Contributor

Related with ipfs/js-ipfs#525 (comment)

@lidel wanna help us achieve both?

@daviddias daviddias added backlog and removed ready labels Jan 29, 2017
@lidel
Copy link
Contributor Author

lidel commented Jan 30, 2017

I am sorry, I have limited free time and need to prioritize my activities.
This is not a blocker for me, just an inconvenience, so it does not have high priority :-)
WebExtension migration is my main focus right now.

@vasco-santos
Copy link
Contributor

Hello @diasdavid

I read the Issue 525, particularly your comment previously referenced and the PRs associated. I have been analyzing how to implement this feature for both js-ipfs and js-ipfs-api.

  1. In the js-ipfs, I am thinking on the following:

https://github.com/ipfs/js-ipfs/blob/master/src/core/index.js#L131
After this line, add an export to the util object:

exports.util = {
  Buffer: Buffer,
  crypto: crypto,
  dagPB: dagPB,
  dagCBOR: dagCBOR,
  multibase: multibase,
  multihash: multihash,
  stream: stream
}

Usage:

const ipfs = require('ipfs')
const util = ipfs.util

even cleaner using imports:

import IPFS, { util } from 'ipfs';

In spite of being possible to provide the util object inside the class using this.util, I consider this to be the easiest way to provide the util object to js-ipfs consumer.

  1. For the js-ipfs-api repo, I wanted to follow the same strategy for consistency. However, this repo already has Utility Functions provided.

Taking into account the second point, I think for consistency among repos, I should expose the util inside the class for the js-ipfs and add it to Utility Functions for the js-ipfs-api.

What do you think?

@daviddias
Copy link
Contributor

We should call it types instead of util. Another option being or util.types

@vasco-santos
Copy link
Contributor

vasco-santos commented Apr 3, 2018

So, in the js-ipfs already exists the types in the class instance. Can I add the remaining ones in there?

For the js-ipfs-api repo, I will create the types as in the other repo.

@daviddias
Copy link
Contributor

Go for it :)

@ghost ghost removed the in progress label Apr 5, 2018
@alanshaw
Copy link
Contributor

For ipfs-postmsg-proxy I'm having to add ipld-dag-cbor, multibase, multihashes, libp2p-crypto and is-ipfs as direct dependencies although the library does not use them and has no guarantee that any consumers of the library use them.

It also doesn't depend on js-ipfs so these dependencies definitely would not normally be in the bundle unless the consumer app uses them. I feel like this is bloating the bundle size unnecessarily.

The exact same thing happened in the PR to js-ipfs-api.

In the case of js-ipfs, it is worth it for convenience since they are already bundled, but adding unused dependencies doesn't feel right and is not in the spirit of the issue title "Provide access to bundled libraries when run in browser".

@vasco-santos
Copy link
Contributor

Hey @alanshaw

I agree that there are several cases where the bundle size was increased with no benefit. That's why I mentioned the increase in the bundle size on the PR. However, I think that if we support this on js-ipfs, we have to support this on js-ipfs-api for consistency. Therefore, we should support it in both, or in none of them.

Moreover, I also suggested removing the Buffer in a breaking change, since it is automatically added by the bundlers nowadays. This way, we could also decrease the bundle size removing it.

@daviddias
Copy link
Contributor

We need that tree-shaking, it will come once ESM support is everywhere :)

@mitra42
Copy link

mitra42 commented Dec 4, 2018

I just found this, I'm curious why it was defined based on an instance of ipfs, rather than something static since it points at the statically defined types.

My specific use case is to do something like foo instanceof ipfs.types.CID but it happens in static functions or places where the ipfs instance isn't passed as a variable, i.e. where I don't easily have access to the "ipfs" variable, but have easy access to the result of
IPFS = require('ipfs'),

The current code does CID=require('cids') solely to allow this type check, which seems exactly the kind of example this change was intended to avoid.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants