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

fix: remove non default ipld formats in the browser #1980

Merged
merged 4 commits into from
Apr 12, 2019

Conversation

hugomrdias
Copy link
Member

This PR removes non default ipld formats in the browser, these formats are extremely big, some of them aren't finished and are not commonly used in the wild.
Considering that we now have options to add support for them in the config passed to the IPFS constructor in both sync and async ways, we should let the developer choose which formats to support and make their bundle smaller and faster to load.

Documentation was added to explain how to handle the ipld config in several ways.

@ghost ghost assigned hugomrdias Apr 8, 2019
@ghost ghost added the status/in-progress In progress label Apr 8, 2019
Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

LGTM, but lets cc @olizilla as this impacts IPLD Explorer in Web UI


To add support for other formats we provide two options, one sync and another async.

Examples for the sync option:
Copy link
Member

@olizilla olizilla Apr 9, 2019

Choose a reason for hiding this comment

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

This is the wonderful documenting @hugomrdias, I love it!

Copy link
Member

@olizilla olizilla left a comment

Choose a reason for hiding this comment

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

This is rad. 👍 I look forward to tweaking IPLD explorer for this change.

The only thing that could make it better is a mechanim that would allow the user to provided custom implementations of dag-pb and dag-cbor and dag-raw. I don't know if that's a feature that anyone wants, but as js-ipld will throw if it finds duplicate handlers for a single format, and the defaults are added regardless, it's the only issue that occured to me as I read the (beautiful) code.

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.

🚀

README.md Outdated Show resolved Hide resolved
@hugomrdias
Copy link
Member Author

This is rad. 👍 I look forward to tweaking IPLD explorer for this change.

The only thing that could make it better is a mechanim that would allow the user to provided custom implementations of dag-pb and dag-cbor and dag-raw. I don't know if that's a feature that anyone wants, but as js-ipld will throw if it finds duplicate handlers for a single format, and the defaults are added regardless, it's the only issue that occured to me as I read the (beautiful) code.

They actually are not added regardless in IPLD, but i forced it on IPFS side with this PR. merge-options doesn't concat arrays by default so if the user does this:

new IPLD({
	formats: [require('ipld-git'])
})

He only gets git and NOT pb, cbor and raw but now with IPFS the user gets pb, cbor, raw and git.
We can remove this behaviour and keep it like IPLD, that way the user can provide custom cbor implementation but he will be forced to add all formats to the array. It's a tradeoff but i think this should be discussed in IPLD solved there then we can do whatever changes here.

Regarding your comment on custom dag-pb it's a design problem format and addFormat work the same way internally but they don't teach the user that we map string identifiers or multicodec constants to implementations like git-raw -> ipld-git.
One solution would be to not rely on the options passed to the constructor to add pb, cbor and raw but dynamically default to them here if everything else fails to load the code needed. /cc @vmx

@alanshaw
Copy link
Member

alanshaw commented Apr 9, 2019

I'm 👍 on making the bundle size smaller and I'm 👍 on providing a build of IPFS that doesn't have these IPLD formats for developers who do not use and will never use other IPLD formats.

I'm 👎 on removing them by default because it won't help IPLD adoption. It raises a barrier for people experimenting with IPLD that the project just doesn't need at this point in it's life. People should just be able to fetch a git commit or an ethereum block from their web app if they want to and experience the magic without having to learn about IPLD format modules and how to programmatically load them.

The documentation in this PR is great, but do you think that someone putting a script tag on their page and trying to dag.get a CID for a git commit is going to come back to the js-ipfs README, find that section, read it all and then update their code? I think that's a big ask.

In general the default IPFS you get when you require or download from a CDN should be the full fat version, and as developers get more knowledgeable in the IPFS ecosystem they should be able to tailor the build for their needs.

IMHO the solution to the problem of the IPLD modules being big or incomplete is not to remove them from IPFS (or even IPLD for that matter) but to fix them. I feel like we're focusing our energies in the wrong place.

That said, what if we iterate on this PR and add an additional browser build that doesn't include the more exotic IPLD formats and a require('ipfs/lite') so people bundling their own can also get the benefits? Would that be a reasonable compromise?

There seems to be a lot of support for this PR so I'm not going to stand in the way of merging this if the @ipfs/javascript-team team really believes it's necessary. If you're in agreement with the compromise I suggested above (or something similar) then please vote with a 👍 and if you disagree and would prefer to merge this as is then 👎 on this comment and I'll get it done.

@hugomrdias
Copy link
Member Author

I'm 👎 on removing them by default because it won't help IPLD adoption. It raises a barrier for people experimenting with IPLD that the project just doesn't need at this point in it's life. People should just be able to fetch a git commit or an ethereum block from their web app if they want to and experience the magic without having to learn about IPLD format modules and how to programmatically load them.

The documentation in this PR is great, but do you think that someone putting a script tag on their page and trying to dag.get a CID for a git commit is going to come back to the js-ipfs README, find that section, read it all and then update their code? I think that's a big ask.

This is basically the standard problem of magic vs control, IMO we should go for control because were are coding the build blocks of the decentralized web, magic should come in with the devs building apps and frameworks with our code. These devs will want control over magic.
Still i agreed we should also think about other user types and for them we should look at how other projects do it.
One solution i really like is how react and jest do it, it's both simple and elegant, we should just write awesome errors messages that explain the problem and present the solution with links to the documentation.

In general the default IPFS you get when you require or download from a CDN should be the full fat version, and as developers get more knowledgeable in the IPFS ecosystem they should be able to tailor the build for their needs.

We can make our bundle the one used when loaded using UNPKG with a script tag, include all the formats easily using .aegir.js to add an alias for webpack.

IMHO the solution to the problem of the IPLD modules being big or incomplete is not to remove them from IPFS (or even IPLD for that matter) but to fix them. I feel like we're focusing our energies in the wrong place.

Agreed, still i don't think we should force all users to include them, goes back to control over magic.

That said, what if we iterate on this PR and add an additional browser build that doesn't include the more exotic IPLD formats and a require('ipfs/lite') so people bundling their own can also get the benefits? Would that be a reasonable compromise?

For me this would raise more questions in the user mind than a well written error message with a link to the simple solution.
IMO we should only have one bundle for the CDN and one entry point in the code, more than that would create fragmentation in the issues, complexity in the code, etc.

@hugomrdias hugomrdias requested a review from jacobheun April 9, 2019 10:45
@olizilla
Copy link
Member

olizilla commented Apr 9, 2019

Good point! I'm very much favour of fixing the IPLD formats. Alan's point about focusing our efforts on where they can do most good is important. IPLD Explorer and anyone wanting to experience the magic wont get much futher than files, cbor and git today, regardless of if they are bundled by default, or have clear instructions on how to add them.

But perf and bundle size is important too, and the browser runtime is arguably more important to us than node is. I'm in favour of not supporting all formats by default in js-ipfs.

I'd be in favour of a compromise like including raw, cbor, pb andipld-git by default so we could show the cross-system magic without config, whilst dropping eth-*, bitcoin and zcash, along with adding great docs and clear errors with instructions on how to add them.

My real dream......is that we pull out the `dag` api entirely to it's own tool called `ipld` and we focus the ipfs story on files, and then follow up with "you want arbitrary, cross-system, merkle graphs with friends!? we got you! try out the `ipld` tool.

@mikeal
Copy link
Contributor

mikeal commented Apr 9, 2019

FYI, we’re working on a new JS stack for IPLD authoring https://github.com/ipld/js-ipld-stack

One change is that the Block interface dynamically loads the necessary codec using a new library called get-codec. My intention is to keep this module very small and have different entry points for Node.js and browsers, with the browser version dynamically loading every codec (except raw cause it’s only like 4 lines in the new interface). This will remove the need for consumers to do dynamic loading of these codecs.

Update: here’s a more complete sample of what I’m thinking for get-codec https://github.com/ipld/js-ipld-stack/tree/master/src/get-codec

@alanshaw
Copy link
Member

We can make our bundle the one used when loaded using UNPKG with a script tag, include all the formats easily using .aegir.js to add an alias for webpack.

I think that's a good compromise - can we do this please @hugomrdias?

To confirm:

  • In Node.js require('ipfs')
    • all IPLD formats included
  • In browser application bundle require('ipfs') bundled with webpack/browserify/etc.
    • only ipld-dag-pb, ipld-dag-cbor and ipld-raw included
  • CDN bundle <script src="https://unpkg.com/ipfs/dist/index.min.js"></script>
    • all IPLD formats included

@alanshaw
Copy link
Member

FYI, we’re working on a new JS stack for IPLD authoring https://github.com/ipld/js-ipld-stack

@mikeal that's rad! I'm excited for get-codec.

Did you see #1830? Similar kind of idea but loading directly from IPFS. It had significant push back (see the PR comments) so my next step was to pull it out into a module people could use, similar to get-codec but I didn't get round to it yet 😢.

@hugomrdias
Copy link
Member Author

hugomrdias commented Apr 12, 2019

We can make our bundle the one used when loaded using UNPKG with a script tag, include all the formats easily using .aegir.js to add an alias for webpack.

I think that's a good compromise - can we do this please @hugomrdias?

Yes

To confirm:

  • In Node.js require('ipfs')

    • all IPLD formats included
  • In browser application bundle require('ipfs') bundled with webpack/browserify/etc.

    • only ipld-dag-pb, ipld-dag-cbor and ipld-raw included
  • CDN bundle <script src="https://unpkg.com/ipfs/dist/index.min.js"></script>

    • all IPLD formats included

Yes, correct.

@alanshaw alanshaw force-pushed the fix/bundle-size-ipld branch from eafef16 to 6bfca39 Compare April 12, 2019 11:49
@ghost ghost assigned alanshaw Apr 12, 2019
@alanshaw
Copy link
Member

@hugomrdias I've rebased for green CI

@alanshaw alanshaw merged commit 4376121 into master Apr 12, 2019
@ghost ghost removed the status/in-progress In progress label Apr 12, 2019
@alanshaw alanshaw deleted the fix/bundle-size-ipld branch April 12, 2019 13:10
@daviddias
Copy link
Member

Commenting now upon request

Folks, this seems like a good win when it comes to bundle size. Did you get the chance to make sure that the experience of https://github.com/ipfs/js-ipfs/tree/master/examples/traverse-ipld-graphs && https://github.com/ipfs/js-ipfs/tree/master/examples/explore-ethereum-blockchain is still flawless?

Also, I see that no tests were added to check the remaining formats integration. Was this intentional?

@alanshaw
Copy link
Member

Did you get the chance to make sure that the experience of https://github.com/ipfs/js-ipfs/tree/master/examples/traverse-ipld-graphs && https://github.com/ipfs/js-ipfs/tree/master/examples/explore-ethereum-blockchain is still flawless?

Since these are Node.js based examples they should be fine - in Node.js all the formats are still included (as always). This would also have been checked as part of the release process for 0.35.

Also, I see that no tests were added to check the remaining formats integration. Was this intentional?

No that would be a good test to add. We don't do any testing with the built CDN bundle and we really should.

@hugomrdias
Copy link
Member Author

@alanshaw our karma tests, effectively test our "built CDN bundle" just doesn't run terserjs to increase speed. There is an issue in aegir to add terserjs to karma to be 100% the same thing.

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.

8 participants