-
Notifications
You must be signed in to change notification settings - Fork 124
feat: Provide access to bundled libraries when in browser #252
feat: Provide access to bundled libraries when in browser #252
Conversation
SPEC/TYPES.md
Outdated
- [`ipfs.types.multibase`](https://github.com/multiformats/multibase) | ||
- [`ipfs.types.multihash`](https://github.com/multiformats/js-multihash) | ||
- [`ipfs.types.CID`](https://github.com/ipld/js-cid) | ||
- [`ipfs.types.crypto`](https://github.com/libp2p/js-libp2p-crypto) |
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.
Not a type.
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.
Agreed. Should I create util
scope for encompassing this, as well as isIPFS
, since they are mentioned in the issue?
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.
Good idea!
SPEC/TYPES.md
Outdated
- [`ipfs.types.crypto`](https://github.com/libp2p/js-libp2p-crypto) | ||
- [`ipfs.types.dagPB`](https://github.com/ipld/js-ipld-dag-pb) | ||
- [`ipfs.types.dagCBOR`](https://github.com/ipld/js-ipld-dag-cbor) | ||
- [`ipfs.types.isIPFS`](https://github.com/ipfs-shipyard/is-ipfs) |
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.
Not a type.
SPEC/TYPES.md
Outdated
- [`ipfs.types.CID`](https://github.com/ipld/js-cid) | ||
- [`ipfs.types.crypto`](https://github.com/libp2p/js-libp2p-crypto) | ||
- [`ipfs.types.dagPB`](https://github.com/ipld/js-ipld-dag-pb) | ||
- [`ipfs.types.dagCBOR`](https://github.com/ipld/js-ipld-dag-cbor) |
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.
perhaps scope these under IPLD and add all the ones that are supported today?
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 agree with your idea. However, I would have to add ipld/js-cid
too. As a consequence, a breaking change would be created in js-ipfs
core/index.js#L62. What do you think?
Regarding adding the other ones, if we decide for creating the specific scope, I think we may add them as well.
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.
That's a valid point, thanks for thinking of that. Let's keep it as it is then.
164a133
to
0cf94b8
Compare
Done @diasdavid 😄 I also updated the other PRs accordingly. |
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.
❤️ it!
const dirtyChai = require('dirty-chai') | ||
const expect = chai.expect | ||
chai.use(dirtyChai) | ||
util |
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.
undefined
?
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.
That util was supposed to be inside describe
. How should I proceed @diasdavid ? A new PR fixing this typo?
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.
@vasco-santos yes please.
const expect = chai.expect | ||
chai.use(dirtyChai) | ||
|
||
describe('.types', function () { |
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.
Are these runnable? There's no module.exports = (common) => {
wrapper and no entry in js/src/index.js
for them.
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.
Well noticed, I have to update this with the other typo.
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.
@vasco-santos did you open a new PR?
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.
It is created now 🙂
This PR aims to update the spec, as a result of the Issue 406.
According to the following PRs:
js-ipfs
: PR 1297js-ipfs-api
: PR 732