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.

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

feat: replace redux-bundle with ipfs-provider #1563

Merged
merged 8 commits into from
Jul 31, 2020
Merged

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Jul 28, 2020

This rips out ipfs-redux-bundle and substitutes it with ipfs-provider so that we can update ipfs-http-client as needed without having to do it through ipfs-redux-bundle.

Note: I keep getting Error: Cannot find module 'ky/umd' when running npm test locally but so far have no idea what causes it. Edit: Turns out I had to do npm build first.

@Gozala
Copy link
Contributor Author

Gozala commented Jul 28, 2020

@rafaelramalho19, @lidel can one (or both) of you please review / land this ?

@Gozala

This comment has been minimized.

@Gozala

This comment has been minimized.

@Gozala
Copy link
Contributor Author

Gozala commented Jul 28, 2020

I have figured out everything. Now it works as expected and passes all the tests.

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.

Thank you @Gozala, this will make http client upgrades muuuuuch easier!

Works as expected in browser and ipfs-desktop, but there is a small UI regression on Status page.
Instead of the URL of detected API (apiAddress) the name of provider is displayed:

2020-07-29_14-01

src/bundles/ipfs-provider.js Outdated Show resolved Hide resolved
src/bundles/ipfs-provider.js Outdated Show resolved Hide resolved
src/bundles/ipfs-provider.js Outdated Show resolved Hide resolved

try {
const result = await getIpfs({
// @ts-ignore - TS can't seem to infer connectionTest option
Copy link
Member

Choose a reason for hiding this comment

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

Q: would adding TS to ipfs-provider help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, but I would not want to block on that

@Gozala

This comment has been minimized.

@Gozala Gozala requested a review from lidel July 29, 2020 21:36
@Gozala
Copy link
Contributor Author

Gozala commented Jul 29, 2020

@lidel I have fixed the issue reported in #1563 (review)

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!

@olizilla @rafaelramalho19 @hacdias any concerns on your end, or ok to merge this?

},
loadHttpClientModule: () => HttpClient,
providers: [
providers.httpClient({ apiAddress })
Copy link
Member

@lidel lidel Jul 30, 2020

Choose a reason for hiding this comment

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

Q: what happens when user loads webui via API port in go-ipfs, but used custom port?

Namely, webui should "just work" when loaded from http://127.0.0.1:5002/webui (note custom port 5002).

Old code tried current origin and logged:

Trying ipfs-api at current origin /ip4/127.0.0.1/tcp/5002/http

What we need is something like PoC below (try potentially customized apiAddress, if that fails try current origin's root):

Suggested change
providers.httpClient({ apiAddress })
providers.httpClient({ apiAddress }),
providers.httpClient({ apiAddress: window.location.origin }),

@Gozala thoughts?

Copy link
Contributor Author

@Gozala Gozala Jul 30, 2020

Choose a reason for hiding this comment

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

I was under impression that ipfs-provider does that already (inline below):

  // Current origin is not localhost:5001 so try with current origin info
  const { location } = root
  if (location && !(location.port === '5001' && location.hostname.match(/^127.0.0.1$|^localhost$/))) {
    const origin = new URL(location.origin)
    origin.pathname = '/'
    const res = await maybeApi({
      apiAddress: origin.toString(),
      connectionTest,
      httpClient
    })
    if (res) return res
  }

As an aside it would be nice if ipfs-provider had an API that was easier to introspect, e.g.

const provider = new IPFSProvider({
    loadHttpClientModule: () => window.IpfsHttpClient,
    loadJsIpfsModule: () => window.Ipfs,
    providers: [ /* see Usage below */ ]
})

console.log(provider.strategy) // => prints out ever strategy that `provide()` will try until one succeeds.

const { ipfs } = await provider.provide()

Copy link
Member

@lidel lidel Jul 31, 2020

Choose a reason for hiding this comment

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

Ah, right, the fallback to origin kicks in if apiAddress is undefined:

// Explicit custom apiAddress provided. Only try that.
  if (apiAddress) {
    return maybeApi({ apiAddress, connectionTest, httpClient })
  }
  // fallback to origin
  // Current origin is not localhost:5001 so try with current origin info
  // ...

In that case disregard my comment :)

(strategy inspector for ipfs-provider is a good idea – feel free to PR, you should have write access to the repo too)

@lidel
Copy link
Member

lidel commented Jul 31, 2020

LGTM, thank you @Gozala!

@rafaelramalho19 @jessicaschilling FYSA I am merging this so #1571 can switch to the new logic

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

Successfully merging this pull request may close these issues.

3 participants