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.

Sign up for GitHub

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

Jump to bottom

fix: use @ipld/dag-pb instead of ipld-dag-pb #116

Merged
merged 26 commits into from
Jul 9, 2021
Merged

fix: use @ipld/dag-pb instead of ipld-dag-pb #116

merged 26 commits into from
Jul 9, 2021

Conversation

vmx
Copy link
Member

@vmx vmx commented Feb 16, 2021 •
edited
Loading

Use the rewrite of the dag-pb codec.

@vmx vmx requested a review from achingbrain February 16, 2021 14:38
@rvagg
Copy link
Member

rvagg commented Feb 16, 2021 •
edited
Loading

Surprisingly easy, maybe deceptively so? I hope not!
The only catch might be Tsize as discussed in ipld/js-dag-pb#5, either we don't have test cases here that assert expected CIDs for cases where Tsize might be defaulted to 0 in the old stack but missing in the new, or @ipld/dag-pb is doing the same thing by omitting it. I don't think this is the case though, the old JS library seems to be defaulting to 0 where you omit it. So we should resolve this. Either just make prepare() more of a hack by adding it in. Or, ideally, fix it at the spec level so we have consistent behaviour between Go and JS (and other) implementations.

@achingbrain
Copy link
Member

This is great, thanks for submitting it. Could you please open a PR to js-ipfs that uses this branch? Before merging it would be great to see the effect on bundle size, performance, and compatibility. If it's 'not much' then it shows we can roll this upgrade out piecemeal instead of in a big bang.

@vmx vmx force-pushed the js-dag-pb branch 2 times, most recently from 6241bb3 to 3871d5e Compare February 18, 2021 16:27
@vmx vmx force-pushed the js-dag-pb branch 2 times, most recently from 7c678df to d1f7546 Compare February 24, 2021 14:52
@rvagg
Copy link
Member

rvagg commented Feb 25, 2021

I came back here to comment on the js-multiformats CID vs js-cid CID problem arising out of @ipld/dag-pb decoding, but it looks like this PR has expanded to a full CID replacement effort! How's that going @vmx?

@vmx vmx mentioned this pull request Feb 25, 2021
Closed
@vmx vmx mentioned this pull request Mar 9, 2021
Closed
@BigLep BigLep added the status/in-progress In progress label Mar 15, 2021
@vmx
Copy link
Member Author

vmx commented Apr 7, 2021

This version of the PR is using the released version of js-dag-cbor, which doesn't work properly with TypeScript types. I've a commit locally that uses ipld/js-dag-cbor#18, but as local hackery is needed (using it as a Git dependency won't work) I haven't pushed that commit.

The exporter browser tests fail, due to the published version of js-dag-cbor using multihash v4 and this PR is using v6. This leads to two versions of the module which causes problems in the browser bundle. If ipld/js-dag-cbor#18 is used, the tests pass.

@lidel
Copy link
Member

lidel commented Apr 19, 2021

Feedback from triage: TS needs to land before this.

vmx added 11 commits April 22, 2021 19:01
@vmx @rvagg
fix: use @ipld/dag-pb instead of ipld-dag-pb
2f24a1c 
Use the rewrite of the dag-pb codec.
@vmx @rvagg
WIP: all in using js-multiformats for everying
8779482 
 - One test suite is commented out
 - bignumber.js was added as dependency to work around a type bug
 - some types should work, but don't, hence there are too many `// @ts-ignores`
 - webworker tests are broken, due to a bug in js-multiformats
@vmx @rvagg
feat: replace hashAlg option with actual hasher
9663d14 
Instead of passing it a number, you now pass in a hasher implementation.
@vmx @rvagg
fixup with WIP commit: things almost work, make it fully work once js…
ce3d324 
…-multiformats pr is merged
@vmx @rvagg
chore: format builder.spec.js properly
e746040
@vmx @rvagg
fix: fix typescript typechecking errors
052eb3f
@vmx @rvagg
fix: more typescript type fixes
5e250c8
@vmx @rvagg
fix: more typescript type fixes
566e5f9
@vmx @rvagg
chore: update dependencies
1f898d2 
A few `@ts-ignore`s are needed until js-dag-cbor is updated to work
properly with CommonJS and TypeScript types.
@vmx @rvagg
fix: use correct size of the node
11d8d74
@vmx @rvagg
chore: fix linter errors
75ac9ff
@olizilla olizilla mentioned this pull request May 7, 2021
Closed
@olizilla olizilla mentioned this pull request May 10, 2021
Closed
@lidel lidel added the status/blocked Unable to be worked further until needs are met label May 10, 2021
@lidel
Copy link
Member

lidel commented May 10, 2021

Triage note: mostly done, but waiting for multiformats work to bubble up to js-ipfs.

@lidel lidel removed the status/blocked Unable to be worked further until needs are met label May 17, 2021
@BigLep
Copy link

BigLep commented May 24, 2021

Assigned to @achingbrain given he has the context to make meaningful progress in a tribute week. It can get picked up when he is on Tribute rotation.

@achingbrain achingbrain mentioned this pull request May 26, 2021
Closed
20 tasks
Gozala
Gozala approved these changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

Other than question already addressed at @rvagg (about nameless links) it looks good to me.

@rvagg rvagg mentioned this pull request Jun 24, 2021
Merged
@rvagg
Copy link
Member

rvagg commented Jun 24, 2021

@achingbrain @Gozala I've pushed a commit to this branch that uses the new createNode() and createLink() factories from dag-pb as per ipld/js-dag-pb#20. After implementing it here it seems like a win to me - less magic property name futzing and fewer lines of code. It almost retires the need for prepare() in fact. If you think it's a negative then let me know, it can be backed out.

8e9a01c

@lidel lidel marked this pull request as draft July 2, 2021 14:16
@lidel
Copy link
Member

lidel commented Jul 2, 2021

Triage note: waiting for green CI for js-ipfs

@achingbrain
chore: add missing deps
a794648 
chore: remove pin/preload options and use correct aegir check command

chore: update interface-blockstore version

chore: update deps

chore: update deps and fix linting
@achingbrain achingbrain marked this pull request as ready for review July 9, 2021 14:19
@achingbrain achingbrain merged commit bab1985 into master Jul 9, 2021
@achingbrain achingbrain deleted the js-dag-pb branch July 9, 2021 14:30
@olizilla
Copy link
Member

olizilla commented Jul 9, 2021

GO TEAM 🎉

github-actions bot pushed a commit that referenced this pull request Apr 26, 2022
@semantic-release-bot
chore(release): 1.0.0 [skip ci]
930f1fb 
## ipfs-unixfs-v1.0.0 (2022-04-26)

### ⚠ BREAKING CHANGES

* ./src/dir-sharded is not in the exports map so cannot be imported

Co-authored-by: Alex Potsides <[email protected]>
* uses new multiformats stack and takes a blockservice instead of the block api

Co-authored-by: Rod Vagg <[email protected]>
Co-authored-by: achingbrain <[email protected]>
* switches to named exports
* types are now included with all unixfs modules
* does not convert input to node Buffers any more, uses Uint8Arrays instead

### Features

* add types ([#114](#114)) ([ca26353](ca26353))

### Bug Fixes

* add pbjs namespace ([#145](#145)) ([dd26b92](dd26b92))
* declare types in .ts files ([#168](#168)) ([76ec6e5](76ec6e5))
* ignore high mode bits passed to constructor ([#53](#53)) ([8e8d83d](8e8d83d))
* ignore undefined values in options ([#173](#173)) ([200dff3](200dff3))
* individual packages can use npm 6 ([#167](#167)) ([2b429cc](2b429cc))
* publish with types in package.json ([#166](#166)) ([0318c98](0318c98))
* remove node globals ([#52](#52)) ([5414412](5414412))
* replace node buffers with uint8arrays ([#69](#69)) ([8a5aed2](8a5aed2)), closes [#66](#66)
* types with ipjs build ([#165](#165)) ([fea85b5](fea85b5))
* use @ipld/dag-pb instead of ipld-dag-pb ([#116](#116)) ([bab1985](bab1985))

### Trivial Changes

* add travis file and configure build scripts ([5a25c87](5a25c87))
* consolidate .gitignore files ([b05e468](b05e468))
* declare interface types in .d.ts file ([#122](#122)) ([eaa8449](eaa8449))
* dep updates ([cf9480b](cf9480b))
* **deps-dev:** bump aegir from 26.0.0 to 28.1.0 ([#86](#86)) ([87541c7](87541c7))
* **deps-dev:** bump aegir from 28.2.0 to 29.2.2 ([#101](#101)) ([010ab47](010ab47))
* exclude docs and tests from npm package ([63b8ba0](63b8ba0))
* move files into packages folder ([943be9d](943be9d))
* publish ([5203595](5203595))
* publish ([0f9092e](0f9092e))
* publish ([2713329](2713329))
* publish ([35e2059](35e2059))
* publish ([137a4ad](137a4ad))
* publish ([f173850](f173850))
* publish ([2ea467f](2ea467f))
* publish ([dedbd82](dedbd82))
* publish ([dc2d400](dc2d400))
* publish ([27d57df](27d57df))
* publish ([5ccac2f](5ccac2f))
* publish ([172548b](172548b))
* publish ([9a2b5f2](9a2b5f2))
* publish ([e57ba16](e57ba16))
* publish ([9e8f077](9e8f077))
* publish ([22e29bb](22e29bb))
* publish ([dabbb48](dabbb48))
* publish ([32e5165](32e5165))
* publish ([5d3f4bd](5d3f4bd))
* publish ([49c8c54](49c8c54))
* publish ([db2c878](db2c878))
* publish ([9237250](9237250))
* remove changes from readme ([7c727ef](7c727ef))
* remove node buffers from runtime code ([#66](#66)) ([db60a42](db60a42))
* remove redundant test files ([3078608](3078608))
* small readme change ([f45436c](f45436c))
* swap to prereleaseOnly ([efb01ac](efb01ac))
* switch to auto-release ([#208](#208)) ([99386e6](99386e6))
* switch to ESM ([#161](#161)) ([819267f](819267f)), closes [skypackjs/skypack-cdn#171](skypackjs/skypack-cdn#171)
* tighten up input types ([#133](#133)) ([47f295b](47f295b))
* update build scripts ([37d96ee](37d96ee))
* update deps ([#144](#144)) ([f5f5fe4](f5f5fe4))
* update deps ([#78](#78)) ([2bf5d07](2bf5d07))
* update lockfiles ([9d11252](9d11252))
* update package.json scripts and readmes ([bda3717](bda3717))
* update readme ([a012f22](a012f22))
* update readme ([7f93da1](7f93da1))
* update readmes ([#188](#188)) ([273a141](273a141))
* upgrade deps ([3a43e92](3a43e92))
* use npm 7 workspaces instead of lerna bootstrap ([#120](#120)) ([1ceb097](1ceb097))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvagg rvagg rvagg left review comments

@achingbrain achingbrain achingbrain approved these changes

@Gozala Gozala Gozala approved these changes

Assignees

@achingbrain achingbrain

Labels
status/in-progress In progress
Projects
No open projects
IP JS (PL EngRes) v2
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vmx @rvagg @achingbrain @lidel @BigLep @olizilla @Gozala