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: add helia/verified-fetch browser example #285

Merged
merged 25 commits into from
Mar 28, 2024
Merged

Conversation

2color
Copy link
Contributor

@2color 2color commented Feb 6, 2024

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

@SgtPooki
Copy link
Contributor

SgtPooki commented Feb 7, 2024

@2color I added some updates in #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
Copy link
Contributor

achingbrain commented Feb 8, 2024

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

Co-authored-by: Alex Potsides <alex@achingbrain.net>
@2color
Copy link
Contributor Author

2color commented Feb 8, 2024

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

  • Stripping this to the bare minimum necessary
  • Maybe swapping Vite for esbuild for a simpler setup

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
Copy link
Contributor

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
Copy link
Contributor Author

2color commented Feb 9, 2024

@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
Copy link
Contributor Author

2color commented Feb 9, 2024

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

@2color 2color self-assigned this Feb 9, 2024
@achingbrain
Copy link
Contributor

achingbrain commented Feb 9, 2024

@2color take a look at #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
Copy link
Contributor

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 ipfs/helia#426 for more.

achingbrain and others added 2 commits February 10, 2024 21:46
* chore: add test to verified fetch example

Adds a test for JSON-codec data

* chore: test verified fetch during ci

* chore: add tests for other types
@2color
Copy link
Contributor Author

2color commented Feb 12, 2024

@achingbrain ipfs/helia#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
Copy link
Contributor

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

@2color 2color requested a review from achingbrain March 14, 2024 14:18
@2color
Copy link
Contributor Author

2color commented Mar 15, 2024

@achingbrain Friendly ping

@achingbrain
Copy link
Contributor

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?

Copy link
Contributor

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

LGTM, just needs the skipped DAG-CBOR test enabling and for multiple load requests to not trample all over each other (see comment above)

@2color
Copy link
Contributor Author

2color commented Mar 19, 2024

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

* origin/main:
  docs: remove main field from helia-101 example (#169)
  chore(deps-dev): bump webpack-dev-server from 4.15.1 to 5.0.3 (#301)
  docs: running helia-101 examples (#217)
  chore(deps-dev): bump electron from 28.2.7 to 29.1.4 (#303)
@2color
Copy link
Contributor Author

2color commented Mar 27, 2024

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

I implemented that now.

Copy link
Contributor

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

I'm good with merging this. some suggestions for improvement though

examples/helia-browser-verified-fetch/src/App.tsx Outdated Show resolved Hide resolved
examples/helia-browser-verified-fetch/src/Output.tsx Outdated Show resolved Hide resolved
examples/helia-browser-verified-fetch/test/blockstore.js Outdated Show resolved Hide resolved
@SgtPooki
Copy link
Contributor

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 2color merged commit 84a9e04 into main Mar 28, 2024
20 checks passed
@2color 2color deleted the add-verified-fetch branch March 28, 2024 12:49
@2color
Copy link
Contributor Author

2color commented Mar 28, 2024

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

Thrilled to have this example to point folks to.

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