search

ipfs-examples / helia-examples

How to do most anything with your Helia node
https://verified-fetch.vercel.app
64 stars 58 forks source link

feat: add helia/verified-fetch browser example #285

Closed 2color closed 3 months ago

2color commented 5 months ago

This PR adds a new example of the @helia/verified-fetch library in the browser.

SgtPooki commented 5 months ago

@2color I added some updates in https://github.com/ipfs-examples/helia-examples/pull/286 that make using it a little more intuitive.

I'm good with these changes if we can agree on some of the changes in 286, but we should make sure linting works and tests are implemented before merging.

P.S. the monorepo test has been failing for a while. we don't need to fix that in this PR

achingbrain commented 5 months ago

This looks pretty neat, some things to fix before merging, in decreasing importance:

  1. Tests - please check out how the helia-vite example is tested - this should give you enough to crib from that the example loads and clicking the buttons works
  2. The monorepo build is failing - I think this is because there are now multiple versions of eslint plugins present and they are interfering with each other - you may need to downgrade or remove something
  3. Libraries - there's a lot in here that doesn't show how to use @helia/verified-fetch - vite, tailwind, react, postcss, etc. Put yourself in the mind of the time-poor junior developer, if they don't need it to use @helia/verified-fetch, can it be removed to reduce their cognitive load (and our maintenance burden)?
  4. Visuals - it would be most excellent to have a coherent visual identity for these examples. Some of them use the assets from https://github.com/ipfs-shipyard/helia-css - it would be great to continue that in new additions here
2color commented 5 months ago

@achingbrain Agree on the first two points.

Re 3:

Libraries - there's a lot in here that doesn't show how to use @helia/verified-fetch - vite, tailwind, react, postcss, etc. Put yourself in the mind of the time-poor junior developer, if they don't need it to use @helia/verified-fetch, can it be removed to reduce their cognitive load (and our maintenance burden)?

Yeah that's a good point. It's really hard to just use React & Tailwind without importing a whole morass of other stuff. Practically though, none of our IPFS stuff is friendly for time-poor junior developer.

I'll take a look at:

I still think it'd be useful to have an example like this (in its current state). Maybe we move some of this to the next.js example which we already have and has React.

Visuals - it would be most excellent to have a coherent visual identity for these examples. Some of them use the assets from https://github.com/ipfs-shipyard/helia-css - it would be great to continue that in new additions here

How critical is that? These are examples, why not make them a little fun and colourful?

achingbrain commented 5 months ago

I still think it'd be useful to have an example like this (in its current state).

My concern with this is that we merge it and move on because we are time pressed, then six months down the line there's a regression in a dep that on balance we could show how @helia/verified-fetch works in a browser without, then we (I) burn even more time investigating and fixing it.

This has happened multiple times in this repo and the preceding js-ipfs-examples one, due to how many competing UX frameworks are here, how precarious their dependency trees are and how lax they are about release breaking changes.

Maybe we move some of this to the next.js example which we already have and has React

The focus of the helia-nextjs example is on any custom config you need for next.js, rather than showcasing the Helia API specifically.

The coding conventions/module formats etc between helia and @helia/verified-fetch (ESM, exports maps, etc) are the same so I'm not sure we'd be adding anything to helia-nextjs by doing this.

This example is about @helia/verified-fetch specifically so we should be showcasing the API here.

How critical is that? These are examples, why not make them a little fun and colourful?

I'm down for fun and colourful, stick a hot pink Helia logo in there! But again the maintenance burden is what concerns me. If we end up with 15-20 examples in this repo and are all a bit different, trying to update things when they break can become overwhelming and people tend to give up.

2color commented 5 months ago

@achingbrain I've pushed some changes to simplify the code a fair bit and to remove as many deps as possible.

I saw that the linting failed but I couldn't figure out exactly why: https://github.com/ipfs-examples/helia-examples/actions/runs/7843490975/job/21403945128#step:6:29

I'm getting a different error when running the npm run lint at the root of the repo on my local machine:

helia-nextjs: [13:23:25] eslint [completed]
helia-browser-verified-fetch: [13:23:25] eslint [failed]
helia-browser-verified-fetch: [13:23:25] → BaseConfig » eslint-config-ipfs#overrides[1] » ./ts.js » eslint-config-standard-with-typescript:
helia-browser-verified-fetch:   Configuration for rule "@typescript-eslint/restrict-plus-operands" is invalid:
helia-browser-verified-fetch:   Value {"checkCompoundAssignments":true} should NOT have additional properties.
helia-browser-verified-fetch: BaseConfig » eslint-config-ipfs#overrides[1] » ./ts.js » eslint-config-standard-with-typescript:
helia-browser-verified-fetch:   Configuration for rule "@typescript-eslint/restrict-plus-operands" is invalid:
helia-browser-verified-fetch:   Value {"checkCompoundAssignments":true} should NOT have additional properties.
helia-create-car: [13:23:25] eslint [completed]
Command failed with exit code 1: aegir lint --files **/*.{js,ts,jsx} !**/node_modules/** !**/dist/**
BaseConfig » eslint-config-ipfs#overrides[1] » ./ts.js » eslint-config-standard-with-typescript:
    Configuration for rule "@typescript-eslint/restrict-plus-operands" is invalid:
    Value {"checkCompoundAssignments":true} should NOT have additional properties.

Any pointers as to why and what's the best way to resolve this?

2color commented 5 months ago

@achingbrain I added a test that checks the basic rendering of the page. It seems that all the other tests avoid any network activity for the test (sensible).

Is there anything you'd test here other than some UI elements getting rendered?

achingbrain commented 5 months ago

@2color take a look at https://github.com/ipfs-examples/helia-examples/pull/290 and let me know what you think.

During testing it uses an extended vite config to override the import of the blockstore module during bundling to return a memory blockstore we can pre-load with blocks that would be requested during the tests.

This would let us test that the rendering of each supported content type works as expected without having to go to the network at any point.

I just filled out a simple JSON codec one, but it can be expanded to handle everything else, I think?

achingbrain commented 4 months ago

I added a few more tests to #290 - it seems DAG-CBOR handling is a little wonky, calling .json on the response results in "[object Object] is not valid JSON", see https://github.com/ipfs/helia/pull/426 for more.

2color commented 4 months ago

@achingbrain https://github.com/ipfs/helia/pull/429 broke the tests here (https://github.com/ipfs-examples/helia-examples/actions/runs/7871416533/job/21474667989):

Error: [vite:load-fallback] Could not load /home/runner/work/helia-examples/helia-examples/examples/helia-browser-verified-fetch/test/blockstore.js/identity (imported by ../../node_modules/@helia/utils/dist/src/utils/networked-storage.js): ENOTDIR: not a directory, open '/home/runner/work/helia-examples/helia-examples/examples/helia-browser-verified-fetch/test/blockstore.js/identity'

I'm looking into the helia source to see how to adapt the blockstore override for the tests to pass again. Let me know if you have any pointers.

achingbrain commented 4 months ago

Ha, it broke the hack that allowed testing this example.

2color commented 3 months ago

@achingbrain Friendly ping

achingbrain commented 3 months ago

I tried to load the Uniswap JSON file, nothing much was happening so I tried to load the video, then the Uniswap file appeared.

I guess in-flight requests are not cancelled when a new request is made?

2color commented 3 months ago

I tried to add request cancellation using an AbortController but it seems to not work due to a bug in @helia/verified-fetch.

2color commented 3 months ago

I guess in-flight requests are not cancelled when a new request is made?

I implemented that now.

SgtPooki commented 3 months ago

FYI that the test succeeds locally, and when ran in isolation, but failed in monorepo test: https://github.com/ipfs-examples/helia-examples/actions/runs/8455093871/job/23162709742?pr=285#step:7:464

I kicked off another retry (but saw that it had already been retried). not sure what's going on.

2color commented 3 months ago

Thanks @SgtPooki. I've addressed all the feedback.

Thrilled to have this example to point folks to.