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: download as CAR via the context menu #1837

Merged
merged 3 commits into from Jun 9, 2022
Merged

feat: download as CAR via the context menu #1837

merged 3 commits into from Jun 9, 2022

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Aug 18, 2021

Partially addresses #1798.

Screen Shot 2021-08-18 at 11 56 20.

License: MIT
Signed-off-by: Henrique Dias hacdias@gmail.com

@hacdias hacdias linked an issue Aug 18, 2021 that may be closed by this pull request
@hacdias hacdias added this to In Review in Maintenance Priorities - JS via automation Aug 18, 2021
@hacdias hacdias temporarily deployed to Deploy August 18, 2021 10:01 Inactive
@hacdias hacdias self-assigned this Aug 18, 2021
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.

@rvagg @olizilla @mikeal @alanshaw
Would appreciate some opinionated bikeshed around labels related to DAG export/import in UI (webui/desktop).

Does the above screenshot look sensible?

Mainly, I want this to be consistent with messaging we have in dev docs in other places (are we using "DAG export" or "CAR download" etc)?

@lidel lidel added the need/community-input Needs input from the wider community label Aug 18, 2021
@rvagg
Copy link
Member

rvagg commented Aug 19, 2021

I think this is probably OK, we've put IPFS' "export" inside the "dag" subsection and it's technically correct that it's exporting an entire DAG starting from a root.

We have other place that are front-loading "CAR", like web3.storage which has a putCar() API (though not in docs yet I think), but I'm not sure that's the ideal approach as it would be good to have some wriggle room around transports for moving DAGs around and not box ourselves too tightly into a form. Even the putCar() API in web3.storage just means "give me an object that conforms to the CarReader API, which it then just uses as a simple root provider blockstore (I faked this for percid.va.gg so I could use that API but not deal with CARs).

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 but let's park this until we have a PR with import (#1838) as noted in #1798 (comment)

@lidel lidel added status/blocked Unable to be worked further until needs are met and removed need/community-input Needs input from the wider community labels Sep 2, 2021
@hacdias hacdias moved this from In Review to Parked/Blocked in Maintenance Priorities - JS Sep 2, 2021
@hacdias hacdias temporarily deployed to Deploy June 2, 2022 08:41 Inactive
src/lib/files.js Outdated Show resolved Hide resolved
public/locales/en/app.json Outdated Show resolved Hide resolved
@hacdias hacdias force-pushed the feat/export-dag-car branch 2 times, most recently from 6266f9b to a52f188 Compare June 9, 2022 12:15
@hacdias
Copy link
Member Author

hacdias commented Jun 9, 2022

@lidel I rebased and made the necessary updates to use the new gateway URLs and the nomenclature.

Notes:

  1. I added some of the mechanics we use in feat: use direct links to download all files #1894 such that we don't store the whole file in memory downloading it. We create an artificial a block and click on it.
  2. The gateway download doesn't follow the filename query parameter. I opened an issue in Gateway CAR Download Doesn't Respect filename kubo#9027 about it and made a PR: fix: honour url filename when downloading as CAR/BLOCK kubo#9028
  3. I also don't understand why the CI is failing on "Installing Dependencies" as this PR changes nothing regarding dependencies.

Question:

  1. Are we releasing this without feat: import DAG CAR #1838? feat: import DAG CAR #1838 requires updating Web UI with the new IPFS dependencies. We talked about introducing download only before (Import/export CAR DAG archives #1798 (comment)) but you were hesitant.

@hacdias hacdias requested a review from lidel June 9, 2022 12:18
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias hacdias changed the title feat: export DAG CAR via the context menu feat: download as CAR via the context menu Jun 9, 2022
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, this is unblocked because go-ipfs 0.13 supports ?format=car (release notes) and we now have PR with gateway specs at ipfs/specs#283

Needs small unicode fix below, but I'll apply it shortly.

Unsure why CI deps install fail, probably the usual NPM rot.
Will regenerate package-lock and see if that helps.

src/lib/files.js Outdated Show resolved Hide resolved
@lidel lidel removed the status/blocked Unable to be worked further until needs are met label Jun 9, 2022
@lidel lidel moved this from Parked/Blocked to In Review in Maintenance Priorities - JS Jun 9, 2022
@lidel lidel temporarily deployed to Deploy June 9, 2022 22:47 Inactive
@lidel lidel merged commit ad787dd into main Jun 9, 2022
Maintenance Priorities - JS automation moved this from In Review to Done Jun 9, 2022
@lidel lidel deleted the feat/export-dag-car branch June 9, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Import/export CAR DAG archives
4 participants