Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Dynamically loaded ipfs-http-client #6

Closed
xmaysonnave opened this issue Nov 4, 2019 · 13 comments · Fixed by #8
Closed

Dynamically loaded ipfs-http-client #6

xmaysonnave opened this issue Nov 4, 2019 · 13 comments · Fixed by #8

Comments

@xmaysonnave
Copy link

Dear Friends,
I'm working on a wiki dapp and needed to lighten its overall size.
I load ipfs-http-client only when users need to update their wiki.
I fork ipfs-provider and made the necessary updates:.
https://github.com/xmaysonnave/tiddlywiki-ipfs
is built against
https://github.com/xmaysonnave/ipfs-provider

  • travis runs correctly.
  • I added a .nvmrc as I use nvm.
  • I didn't mock the loader
    Thanks
@autonome
Copy link

autonome commented Nov 4, 2019

Thanks @xmaysonnave! If you are able, can you submit pull-requests to this repo for each of the specific changes you made? It would make it much easier to evaluate and integrate them!

@xmaysonnave
Copy link
Author

Thanks I will provide some PR.
I'm not familiar with the process though.
Thanks for your guidance and understanding while doing.

@autonome
Copy link

autonome commented Nov 5, 2019

Sure, thanks for contributing!

In your fork of this repository, you'll need to make a branch of master with only one of your specific changes applied to it. Then, in the Github UI for that branch you will see a button to create a pull request - click that and follow the prompts to create it.

Then do that again for each of the changes.

Let me know if you have more questions or need help!

@xmaysonnave
Copy link
Author

xmaysonnave commented Nov 5, 2019 via email

@autonome
Copy link

autonome commented Nov 5, 2019 via email

@lidel
Copy link
Member

lidel commented Nov 8, 2019 •
edited

@autonome yes, we want this library to work in browser, electron and backend nodejs.

@xmaysonnave I took a look at changes you made in your fork (diff) and it won't be easy to backport them without making a breaking change of removing js-ipfs-http-client from default bundle.

Another concern is that we don't want to hardcode specific CDN such as unpkg.com in this library.

I think we could change the API to accept getApiClient function, similar to existing getJsIpfs that enables lazy-loading of full js-ipfs node. That way, you could pass your own (loadLibrary) function responsible for fetching js-ipfs-http-client on-demand.

See proposed API changes in #8:

const { ipfs, provider } = await getIpfs({
  tryApi: true,
  getApiClient: () => import('ipfs-http-client')
})
  • tryJsIpfs should be set to true.
  • getApiClient should be a function that returns a promise that resolves with a IpfsHttpClient constructor. This works well with dynamic import(), so you can lazily load js-ipfs-http-client from specified URL when it is needed: 
    getApiClient: () => import('https://unpkg.com/ipfs-http-client@39.0.2/dist/index.min.js')
    Note: If getApiClient is not defined, ipfs-provider expects window.IpfsHttpClient to be present.

@xmaysonnave would the above solve your use case?

@xmaysonnave
Copy link
Author

@lidel Thanks for your detailed feedback

  • I think your proposal to mimic the getJsIpfs behaviour is a good idea
  • I agree that hardcoding a default library is not necessary
  • Not sure yet if dynamic import is able to handle sri however thanks for the pointer as it's quite informative
  • If I'm correct you suggest that removing the bundled js-ipfs-http-client will cause some breaking changes on your side however this is the overall idea of this patch. Even the api is improving it means that at some point two variants of this library will be available. This is a no go us as we would prefer to lazy load the library when needed and avoid any extra weight if not necessary.

Some remarks, correct me if I'm wrong:

  • The default jsIpfsOpts do not contain any default values at this moment however the following instruction:
    opts = Object.assign({}, defaultOpts, opts)
    perform a shallow copy instead of a deep copy. Could you confirm my assumption ?
  • the permissions options is not documented

Final:
Is there any informations about webextension ? I cannot figure out if this feature applies on the browser side or server side and would like to evaluate it.

Thanks for your time.

@lidel
Copy link
Member

lidel commented Nov 11, 2019 •
edited

Not sure yet if dynamic import is able to handle sri however thanks for the pointer as it's quite informative

For users who want SRI there would be support for window.IpfsHttpClient (see my proposal in #8): instead of defining getApiClient one will add <script> dynamically (like you do in loadLibrary in your fork) and bundle will create window.IpfsHttpClient:

<script src="https://cdn.jsdelivr.net/npm/ipfs-http-client@39.0.2/dist/index.min.js" integrity="sha256-x65j2zwTEv1pkZALKuBXmpKP2cMjmqPEQe98pJ3reNY=" crossorigin="anonymous"></script>

removing the bundled js-ipfs-http-client will cause some breaking changes on your side however this is the overall idea of this patch. [..] we would prefer to lazy load the library when needed and avoid any extra weight if not necessary.

Correct, just to be clear I am supportive, we should not force people to bundle http client if they don't use it.
We just need to make sure its intuitive enough.

default jsIpfsOpts do not contain any default values
opts = Object.assign({}, defaultOpts, opts)
perform a shallow copy instead of a deep copy. Could you confirm my assumption ?

I don't know the context of this snppet, but yes, Object.assign is a shallow copy, if you need deep one see examples at MDN

the permissions options is not documented

Not sure what you mean, its defined in Enable window.ipfs section.

Is there any informations about webextension ? I cannot figure out if this feature applies on the browser side or server side and would like to evaluate it.

webextension runtime is a background page of a WebExtension such as IPFS Companion.
It is "browser side" with some additional APIs.

@xmaysonnave
Copy link
Author

Hi Martin,

Thanks for your informations, the cdn link you suggested is definitely faster than the unpkg link I started to use. I'm in the process to improve this PR. Not a top priority though.

1 - remove the default instantiation and let the caller the responsibility to load and inject its Ipfs library

2 - add permissions in default opts and deep copy inner arrays if at some point default values are populated to avoid any further problems :

opts = Object.assign({}, defaultOpts, opts)
opts.permissions = Object.assign({}, defaultOpts.permissions, opts.permissions)
opts.jsIpfsOpts = Object.assign({}, defaultOpts.jsIpfsOpts, opts.jsIpfsOpts)

3 - I'm considering js-ipfs-http-client-lite as it seems to be an interesting undergoing work. Could be the default library as its footprint is really small

4 - Check webextension in firefox, chrome and chromium with Ipfs Companion but no luck as the window.chrome is not available

5 - Made a mistake about permissions, no offense to the package maintainers

6 - about sri I do not declare html tags but use javascript, anyway the pattern in this PR fullfill my need. More elegant and readable code is always welcome.

Warmly

@lidel
Copy link
Member

lidel commented Nov 13, 2019 •
edited

@xmaysonnave thank you for fleshing out this.

Ad 1: If we don't include loadLibrary, I believe most of this work is already done in PR #8, perhaps you could reuse or comment on that PR in case something is missing?

Ad 2: what kinds of problems you had? usually something like if (opts && opts.jsIpfsOpts) is enough and easier to reason about

Ad 4: window.chrome (Chromium) and window.browser (Firefox) is available only in the background page of WebExtension

@xmaysonnave
Copy link
Author

@lidel
Ad 1: You're right it seems good
Ad 2: I need to do my homework here :-)
Ad 4: I figured out yesterday how it works however webext.js process only chrome. Not a problem for me though.
Many thanks for your input. I made a nice step forward.
I'm ready to close this PR as there is nothing to add yet. Please confirm.
Warmly

@lidel
Copy link
Member

lidel commented Nov 19, 2019 •
edited

@xmaysonnave
Ad 4. Firefox supports chrome.* and browser.* so it should not be an issue

Let's keep this open until I find some time to merge #8 and release new version, ideally with #3.

@lidel lidel mentioned this issue Nov 19, 2019
Closed
@xmaysonnave xmaysonnave mentioned this issue Dec 3, 2019
Closed
@lidel lidel mentioned this issue Dec 27, 2019
Merged
@lidel lidel closed this as completed in #8 Apr 22, 2020
@lidel
Copy link
Member

lidel commented Apr 22, 2020

@xmaysonnave apologies for this taking so long – notifications slipped between cracks.

I've just released v1.0.0 which comes with prebuilt browser bundle:
https://github.com/ipfs-shipyard/ipfs-provider#via-prebuilt-browser-bundles

I believe you should be able to swap loadHttpClientModule/loadJsIpfsModule to more advanced version that downloads respective bundles lazily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: lazy load js-ipfs-http-client and async iterator support ipfs-shipyard/ipfs-provider
3 participants
@autonome @lidel @xmaysonnave