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

Migration to the new IPFS JS API with Async Await and Async Iterables #1569

Merged
merged 27 commits into from
Aug 10, 2020

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Jul 29, 2020

Overview

We need to upgrade to new IPFS so we can take advantage of a fix to ipfs/js-ipfs#3029 to unblock #1529. However upgrade end up been complicated by introduces async iterable #1431. And all the involved dependencies that had various conflicts that required updating to get through. Below is high level overview of changes this pull packs:

  • Dependency updates
    • Updates ipfs / ipfs-http-client to latest
      • Updates all uses to take into account async iterables.
      • Updates all uses to take into account ipfs.add / ipfs.addAll API split.
    • Updates ipfsd-ctl to lastest (so that latest js-ipfs / go-ipfs could be passed in)
    • Switches from go-ipfs-dep to go-ipfs (because that is what we do elsewhere and that is what ipfsd-ctl seems to suggest. As far as I understood from talking to @lidel former was used for legacy reasons).
    • Updated bunch of other things because per errors and suggestions from react-scripts
  • filesToStreams got replaced by normalizeFiles function
  • Update bunch of code to use .cid in place of former .hash due to changes in IPFS API.
  • Add bunch of TS via JSDoc comments
    • Seems unrelated, however it was instrumental in figuring out all the places that need to be change.
      • In fact I end up spending a lot more time updating tests, because I could not take advantage of type checker to spot all the API incompatibilities in mocks and all.
    • As per discussion on higher bandwidth channel, I've added those annotations in code paths that I had worked on and added tsconfig.json / npm run check so that files that are annotated get checked.

Fixes #1431

@Gozala Gozala self-assigned this Jul 29, 2020
@Gozala Gozala marked this pull request as draft July 29, 2020 22:09
@Gozala Gozala mentioned this pull request Jul 29, 2020
@lidel lidel changed the title Ipfs update Migration to the new IPFS JS API with Async Await and Async Iterables Aug 6, 2020
@Gozala Gozala marked this pull request as ready for review August 6, 2020 19:39
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.

Thank you for investing time on sorting this out @Gozala!

There is a regression on Files screen in functionalities not covered by E2E tests – mind taking a look?

  1. File size column and pin status are wonky for directories and empty files.
    I suspect API calls used for reading file sizes and pin status changed:

    master this PR
    2020-08-06--23-34-39 2020-08-06--23-35-51
  2. Pinning via context menu does not work (yellow error in UI + one in browser console), most likely for the same reason.

ps. I also found a bug around Download action – it was already in master so not related to this PR – filled #1576

@@ -21,15 +21,16 @@
"bundlesize": "bundlesize",
"eject": "react-scripts eject",
"storybook": "start-storybook -p 9009 -s public",
"build-storybook": "build-storybook -s public"
"build-storybook": "build-storybook -s public",
"check": "tsc --noEmit"
Copy link
Member

Choose a reason for hiding this comment

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

Could this PR include GitHub Action that runs this check on every push and PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added an action, but honestly current coverage is too small to catch much. It does however makes it easy to understand the code you're working with e.g.

image

image

image

image

@Gozala
Copy link
Contributor Author

Gozala commented Aug 7, 2020

There is a regression on Files screen in functionalities not covered by E2E tests – mind taking a look?

Thanks for catching all these!

  1. File size column and pin status are wonky for directories and empty files.
    I suspect API calls used for reading file sizes and pin status changed:

File size was my issue, I have incorrectly entyped type field as 'file' | 'directory' which turns out to be 'file' | 'dir'.
Fixed by 229a66c

Pinning status was fixed by fc6d501. Overlooked that pin status was checked via array.includes which worked for hashes as strings but doesn't for CIDs that are objects.

Pinning via context menu does not work (yellow error in UI + one in browser console), most likely for the same reason.

Fixed by b1b908c. I have no slightest clue what that code supposed to do however. I believe it attempt to flatten array there so I changed check to only do so on arrays, but would be good if someone more knowledgeable could fill out that TODO: to explain what's up.

ps. I also found a bug around Download action – it was already in master so not related to this PR – filled #1576

Strange. I did tested all the context menu actions, including download (on this branch) and it seems to work and download here. It could be that array flattening code was causing problems for downloads too.

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.

Thank you @Gozala, lgtm now 👍

(Download issue was easy to miss, because it occurs only when more than one item is selected – see details in #1576)

Before we merge this, I'd like @rafaelramalho19 to review this too (to be aware of the scope of changes while rebasing other work on top of this, after the merge).

@lidel lidel added the need/maintainers-input Needs input from the current maintainer(s) label Aug 7, 2020
@Gozala Gozala merged commit 16e15a2 into master Aug 10, 2020
@Gozala Gozala deleted the ipfs-update branch August 10, 2020 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/maintainers-input Needs input from the current maintainer(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use ipfs-http-client >42 (refactor with Async Await and Async Iterables)
3 participants