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: dir index html listing #86

Draft
wants to merge 31 commits into
base: main
Choose a base branch
from
Draft

feat: dir index html listing #86

wants to merge 31 commits into from

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented May 23, 2024

  • feat: basic dir-index-html implementation
  • fix: dir-index-html initial link clicking

Title

feat: dir index html listing

Description

Initial implementation of the directory index html listing.

Demo

verified-fetch.index-dir-html.demo.mp4

Notes & open questions

We don't want a templating engine, so code is going to be a bit verbose, but I plan to clean it up before removing this PR from draft.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

…-some-gateway-conformance-tests-that-were-disabled-in-httpsgithubcomipfshelia-verified-fetchpull81
Base automatically changed from 84-fix-infinite-querying-bug-reproducible-on-some-gateway-conformance-tests-that-were-disabled-in-httpsgithubcomipfshelia-verified-fetchpull81 to main May 24, 2024 19:09
@SgtPooki SgtPooki self-assigned this May 24, 2024
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.

Slight concern about generating styled HTML and bundling all the SVG images in this project.
Feels like something that should be done elsewhere – some ideas inline.

Copy link
Member

Choose a reason for hiding this comment

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

Concern

Hm.. with all the CSS and SVG images that take hundreds of kilobytes, and reproducing look and feel for boxo/gateway, this feels like an overkill for this library, and more something that should live in separate package imported by upstream projects like https://github.com/ipfs/service-worker-gateway

Most of users of verified-fetch will use it programmatically to access files, they won't enumerate directories, and if they do, they would prefer deterministic JSON rather than a difficult to parse HTML. Dir listing is something specific to project built on top of this library.

Note that the same approach applies to DAG-CBOR – we are not generating user-friendly HTML in verified-fetch.

Some ideas

I vaguely remember discussing this in the past (apologies, forgot with whom) and the idea was for verified-fetch due its web dev nature to default (if no Accept is passed) to returning more useful JSON representation of UnixFS defined in https://ipld.io/specs/codecs/dag-pb/spec/#logical-format (initial idea was to make it the same DAG-JSON as ipfs dag get QmdC5Hav9zdn2iS75reafXBq1PH4EnqUmoxwoxkS5QtuME so the DX/UX of switching from kubo RPC to verified-fetch is cushioned).

If verified-fetch library returned dir listings as JSON, dir support does not introduce size/dependency overhead for users who only interact with files, and then SW/HTTP gateway could be converting JSON dir listings to user-friendly HTML, and we would not have to maintain any UI elements in this repository.

If we want, a very basic unstyled HTML responses (<ul>) could be produced by verified-fetch, but only if request was done with text/html in Accept.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we do return json, do we have tests for that in gateway conformance? otherwise, I think we should probably optimize for gateway conformance passing now (for feature parity) and improve this lib as gateway conformance has more of a targeted focus for use by verified-fetch

Copy link
Member

@lidel lidel Nov 8, 2024

Choose a reason for hiding this comment

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

No tests for that, its a technical debt I want to pay some day.

Your approach is ok, but if you see a test that makes no sense (overly specific to Kubo) then feel free to skip and fill issue in https://github.com/ipfs/gateway-conformance + ping me to expedite a new release.

btw: i think right now gateway-conformance expects HTML to be always supported, which is a bug imo. Thats fine for now, but in the future we will make conformance tests send explicit Accept header with text/html (like we already do in for DAG-JSON)

@SgtPooki
Copy link
Member Author

SgtPooki commented Nov 7, 2024

we need to create a wrapper module.. simile to how we do @helia/* that augments the functionality of verified-fetch

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.

2 participants