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: use ipld-explorer-components@4.0.0 #119

Merged
merged 28 commits into from
Jul 12, 2023
Merged

feat: use ipld-explorer-components@4.0.0 #119

merged 28 commits into from
Jul 12, 2023

Conversation

SgtPooki
Copy link
Contributor

@SgtPooki SgtPooki commented May 25, 2023

Note that this code is based on changes included in ipfs/ipld-explorer-components#360 and must wait to be merged until we can publish a new ipld-explorer-components lib once that PR is merged.

check out the preview: https://bafybeif65hxysjwygngoahx7v6a2ouzsfi4y24u7ovr4vd5ya4jhq7ngjy.on.fleek.co/

Check out the demo:

explore.ipld.io-2023-05-25-update.mp4

fixes #117


  • Note that fleek build is failing due to being set to node16, RESOLVED

image

package.json Outdated
"ipld-git": "^0.2.3",
"ipld-raw": "^2.0.1",
"ipld-zcash": "^0.4.1",
"ipld-explorer-components": "file:../../ipfs/ipld-explorer-components/ipld-explorer-components-3.0.3.tgz",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will be updated once ipfs/ipld-explorer-components#360 is merged

@@ -1 +1 @@
REACT_APP_VERSION=$npm_package_version
VITE_VERSION=$npm_package_version
Copy link
Contributor Author

Choose a reason for hiding this comment

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

part of vite migration

Comment on lines +11 to +12
<link rel="manifest" href="/manifest.json">
<link rel="shortcut icon" href="/favicon.ico">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

part of vite migration

@@ -36,5 +27,6 @@
To begin the development, run `npm start` or `yarn start`.
To create a production bundle, use `npm run build` or `yarn build`.
-->
<script type="module" src="/src/index.jsx"></script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

part of vite migration

"serve": "vite preview",
"test": "run-s test:unit test:e2e",
"test:unit": "vitest run --environment=node",
"test:unit-playwright": "playwright test -c playwright-ct.config.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not currently used, but available to run unit tests via playwright if we want to normalize on tests.

@@ -11,7 +11,7 @@ export class App extends Component {
doUpdateUrl: PropTypes.func.isRequired,
queryObject: PropTypes.object.isRequired,
registerServiceWorker: PropTypes.func,
route: PropTypes.oneOfType([PropTypes.func, PropTypes.element]).isRequired
route: PropTypes.oneOfType([PropTypes.func, PropTypes.element, PropTypes.elementType]).isRequired
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixes an in-app error regarding prop types mismatch.

// const page = await browser.newPage()
const page = (await browser.pages())[0]
// it('should resolve a dag-pb cid with a path', async () => {
// // Save 3s per run by using the initial page rather than creating a new one!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo: need to migrate this test code to playwright.

@SgtPooki SgtPooki changed the title feat: Update to latest ipld-explorer-components [WIP] feat: Update to latest ipld-explorer-components May 25, 2023
@SgtPooki SgtPooki linked an issue May 26, 2023 that may be closed by this pull request
@SgtPooki
Copy link
Contributor Author

SgtPooki commented May 26, 2023

interesting issue found when doing js-ipfs deprecation: ipfs/js-ipfs#1416

@SgtPooki SgtPooki changed the title [WIP] feat: Update to latest ipld-explorer-components feat: use ipld-explorer-components@4.0.0 Jun 8, 2023
@SgtPooki SgtPooki self-assigned this Jun 8, 2023
@SgtPooki SgtPooki marked this pull request as ready for review June 8, 2023 22:50
@SgtPooki SgtPooki requested review from lidel, hacdias and rvagg June 9, 2023 17:16
Copy link
Collaborator

@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.

Tested latest version from https://bafybeiepdrogejw7eji6qdbq3hpd3ewuuhecyvf4nyofvhntsbyb3w7tqa.on.fleek.co

@SgtPooki looks ok for available content, but trying to fetch something new timeouts bit too early? Lmk if I got things right.

I tried to add a PDF and explore its CID (bafybeibms6xfhs4kx3tun7dg6bjbj5obca7yj43cl5jl3pc2n7muw6ipfm) and the gateway request for the root block was stuck in "Pending":

2023-06-28_02-01

and after a while that request got cancelled:

2023-06-28_02-02

So two problems:

  1. Why is request for single block in Pending? It should fhit gateway asap, before anything else. Are WebTransport requests still suffocating the request queue browser allocates to the page?
  2. Why it cancels (timeouts?) so fast? As long gateway request is in progress or pending, it should not timeout.

@SgtPooki
Copy link
Contributor Author

SgtPooki commented Jul 4, 2023

Why is request for single block in Pending? It should [hit] gateway asap, before anything else. Are WebTransport requests still suffocating the request queue browser allocates to the page?

Helia should not be making any requests except for to the delegatedClient via kubo-rpc-client: https://github.com/ipfs/ipld-explorer-components/blob/ca19310defb84611f55c46b1efd399217ba4a8c6/src/lib/init-helia.ts#L28-L63

Why it cancels (timeouts?) so fast? As long gateway request is in progress or pending, it should not timeout.

Mostly to keep the site responsive and gateway-friendly. I've currently got the timeout set to 30 seconds. I've heard significant feedback from users that explore.ipld.io would spin and spin and they never knew if their content was going to load. I think a much better approach is to fail early, and have the users refresh/retry if they want.

Were you able to fetch your content from http://ipfs.io/ipfs/bafybeibms6xfhs4kx3tun7dg6bjbj5obca7yj43cl5jl3pc2n7muw6ipfm directly? Maybe you removed the content, but I just attempted to access it and it failed after

We shouldn't have users guessing whether a request is going to succeed or not. Also, for failed gateway requests, the gateway keeps the connection open for up to 2 minutes, and sends back almost 1K of data:

image

If we cancel requests that are pending for 30 seconds, we can free up the gateway to do other work, and the user can re-request things if they want. If there is a different timeout value you think would be better or a better way to handle this without significant feature work, then please let me know.


Drawbacks

  • The code is currently set to try the gateways in serial which means if ipfs.io takes more than timeout ms, dweb.link is never attempted. If ipfs.io takes less than timeout ms, then dweb.link is never attempted. These could be made parallel since we removed the non-delegated Helia fetch attempts.

Other things to think about

  • Higher default timeout - Gives users the expectation that something will resolve, when in all likelihood, if it hasn't resolved in 30 seconds, it probably won't. In my experience improving ipld-explorer-components, 30 seconds was a pretty nice spot.
  • Better error message - I looked into displaying a better error message previously, but the way the network requests are hidden so deep in ipld-explorer-components, it was a larger task than I wanted to fit into feat!: remove all old deps ipfs/ipld-explorer-components#360.
    • It would be great to be able to show more details about failed fetch attempts ("failed to load via ipfs.io: Gateway Timeout", "failed to load via dweb.link: Gateway Timeout", "failed to load via local node: request blocked", etc..) and also where we were able to successfully fetch the request from
  • Configurable timeouts - Allow users to specify whether they want the request to timeout (i'm just curiously browsing), or whether they're willing to wait for long enough to load it (I need to actually explore this content for some reason)
  • Stagger parallel gateway requests - We should not broadcast to all gateways at the same time, but we could delay the requests to subsequent gateways by something liketimeout/2 for example.
  • Separate timeouts + staggered requests - We could have a global-get-raw-block-timeout and stagger requests to gateways by ( global-get-raw-block-timeout / N+1 gateways tested)

@SgtPooki SgtPooki requested a review from lidel July 4, 2023 17:51
Copy link
Collaborator

@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 @SgtPooki, failing fast at 30s mark makes sense as a trade-off,
the problem is bad UX on timeout – user adds data to their local node, then tries to inspect it, timeouts, app looks broken:

2023-07-07_19-02

One way to fix this without rewriting the entire thing is to inspect error before printing the red box and if it is timeout/promise cancellation, replace it with friendly "Failed to fetch content in 30s, refresh page to retry or try different CID."? It would be good enough to unblock this PR.

Since ipld-explorer-components@4.0.0 is released, would it be ok to merge this as-is, and fill issues for any remaining work ("Drawbacks", "Other things to think about"), and do it in future PRs (if we have time).

Would be shame if this PR got stuck in review forever: It is already better than the old version! :)

@SgtPooki
Copy link
Contributor Author

SgtPooki commented Jul 7, 2023

Failed to fetch content in 30s, refresh page to retry or try different CID

I see your approval, but I will work on that error message prior to merging.

@SgtPooki
Copy link
Contributor Author

@lidel PR out to address this in ipld-explorer-components: ipfs/ipld-explorer-components#378

@SgtPooki SgtPooki requested a review from a team as a code owner July 12, 2023 17:00
@SgtPooki
Copy link
Contributor Author

@SgtPooki SgtPooki merged commit 65f1fef into master Jul 12, 2023
@SgtPooki SgtPooki deleted the use-vite branch July 12, 2023 17:24
@SgtPooki
Copy link
Contributor Author

Changes deployed on explore.ipld.io.

https://explore.ipld.io/#/explore/bafybeibms6xfhs4kx3tun7dg6bjbj5obca7yj43cl5jl3pc2n7muw6ipfm shows the error message appropriately.

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.

feat: migrate to Helia from js-ipfs code (ipfs-core, ipfs-http-client) Remove Ethereum example?
2 participants