feat: add types #189

achingbrain · 2021-02-11T15:10:54Z

Adds types to all src and tests and fixes all tsc warnings.

codecov · 2021-02-11T15:14:29Z

Codecov Report

Merging #189 (827afd4) into master (fa50119) will decrease coverage by 19.97%.
The diff coverage is 66.66%.

@@             Coverage Diff             @@
##           master     #189       +/-   ##
===========================================
- Coverage   97.57%   77.59%   -19.98%     
===========================================
  Files          16       15        -1     
  Lines         206      366      +160     
===========================================
+ Hits          201      284       +83     
- Misses          5       82       +77

Impacted Files	Coverage Δ
src/dag-link/index.js	`100.00% <ø> (ø)`
src/dag-node/sortLinks.js	`100.00% <ø> (ø)`
src/dag.js	`51.57% <51.57%> (ø)`
src/dag-link/dagLink.js	`100.00% <100.00%> (ø)`
src/dag-link/util.js	`100.00% <100.00%> (ø)`
src/dag-node/addLink.js	`92.30% <100.00%> (ø)`
src/dag-node/dagNode.js	`95.65% <100.00%> (+0.53%)`	⬆️
src/dag-node/rmLink.js	`90.00% <100.00%> (+1.11%)`	⬆️
src/dag-node/toDagLink.js	`100.00% <100.00%> (ø)`
src/genCid.js	`100.00% <100.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa50119...182d0af. Read the comment docs.

achingbrain · 2021-02-11T15:17:54Z

Also of note: removes protons and class-is in favour of protobufjs (because it's used elsewhere in the stack so cuts down on duplication) and just using instanceof because if we have multiple versions of a module in the dep tree, and they are not semver compatible, they are not compatible and we should fix that.

vmx

I only did a brief review. This contains lots of breaking changes. I fear that it would result in a release that breaks many things and it would be a lot of work to fix all those. So I'm not sure if an in-depth review with additional testing makes is time well spent. Or if we e.g. just cut a major release without much thoughts and don't upgrade js-ipld to it (which would mean that js-ipfs would bundle two versions of js-ipld-dag-pb.

I'd prefer if it would just add types and won't do anything else. Especially with the background that we hopefully will use js-dag-pb instead.

vmx · 2021-02-11T16:54:16Z

.travis.yml

+branches:
+ only:
+ - master
+ - /^release\/.*$/


Does it mean that it doesn't run CI if a branch is pushed without doing a PR?

If it's not enabled, you can configure this setting in travis to build every non-pr branch

package.json

vmx · 2021-02-11T17:16:21Z

src/genCid.js

+const cid = async (binaryBlob, userOptions = {}) => {
+ const options = {
+ cidVersion: userOptions.cidVersion == null ? 1 : userOptions.cidVersion,
+ hashAlg: userOptions.hashAlg || defaultHashAlg


That would break if you use the identity hash 0x00.

src/dag-link/dagLink.js

achingbrain · 2021-02-11T18:42:46Z

This should definitely go out as a major release, that's how we're handling all the TS stuff as setups that are already using typecheckers may suddenly find what they thought was the API was not actually the API (intentional breaking changes/API simplifications aside).

we hopefully will use js-dag-pb instead.

I see that as separate to the typing effort.

vmx · 2021-02-11T19:16:48Z

This should definitely go out as a major release.

Yes for sure. Though I don't know if it makes sense to introduce breaking changes like having some properties enumerable that weren't before, which I don't think add much value. I'd keep breaking changes to a minimum in order to minimize potential breakage and time for review and testing.

Anyway, if you think it makes sense I'll spend some time on it to make sure it doesn't break anything in unexpected ways.

achingbrain · 2021-02-12T08:51:34Z

I've made it a draft PR for now, I'll bubble this all the way up to IPFS and we can merge it then (doing the UnixFS importer/exporter types now).

…g generated

vmx

Some type information has no return value defined, sometimes it was even removed. What is the reason for that?

It would be great if you could list all the breaking changes in the commit message, so that it automatically shows up in the changelog.

vmx · 2021-02-15T13:07:43Z

src/dag-link/dagLink.js

+ this.Name = name || ''
+ this.Tsize = size
+ this.Hash = new CID(cid)


Having the values read-only is something dag-pb did (IIRC) from the very beginning. I'm not sure if it is important or not (if it's not important for unixfs, it is probably not).

@rvagg do you have oponions on that?

It's not something that unixfs uses.

Please change it back to the original code. I don't see a reason to make a change that might break things in a subtle way if we don't have to.

vmx · 2021-02-15T13:17:42Z

src/dag-node/dagNode.js

- return serializeDAGNode(this)
+ const buf = serializeDAGNode(this)
+
+ this._serializedSize = buf.length


A test for that fix would be great.

vmx · 2021-02-15T13:18:19Z

src/dag-node/rmLink.js

+ predicate = (/** @type {DAGLink} */ link) => link.Name === nameOrCid
+ } else if (nameOrCid instanceof Uint8Array) {
+ predicate = (/** @type {DAGLink} */ link) => uint8ArrayEquals(link.Hash.bytes, nameOrCid)
+ } else if (CID.isCID(nameOrCid)) {


A test for this fix would be great.

TBH DAGNode.rmLink accepts DAGLink | string | CID so the type checker would object to you passing a Uint8Array in, but not everyone uses a type checker.

vmx · 2021-02-15T13:27:49Z

src/genCid.js

+const cid = async (binaryBlob, userOptions = {}) => {
+ const options = {
+ cidVersion: userOptions.cidVersion == null ? 1 : userOptions.cidVersion,
+ hashAlg: userOptions.hashAlg == null ? defaultHashAlg : userOptions.hashAlg
+ }


Was the old code buggy? I'm asking as I find the old code clearer then the new one, but I might miss some subtle difference.

This change wasn't necessary - I think there was a complaint from tsc but it looks like this wasn't the problem, I can revert if preferred.

I always prefer smaller diffs :) But if it's a hassle, it doesn't bother me much. I was more interested in the reason why.

src/resolver.js

vmx · 2021-02-15T13:46:44Z

src/resolver.js

@@ -37,9 +40,11 @@ exports.resolve = (binaryBlob, path) => {

 // There wasn't even a matching named link
 throw new Error(`Object has no property '${key}'`)
+ } else {


I'm not sure about this change. I usually use if … else if it is a branch whether either one thing or the other happens and the computation goes on with either of them. I you only and if if it is an early return. And in this case it is an early return, the if branch never continues to later parts of the code.

It was mostly to shut the type checker up. Will remove.

vmx · 2021-02-15T13:56:30Z

src/serialize.js

- Links: links
- }))
+ Links: links.map((link) => {
+ return createDagLinkFromB58EncodedHash(link)


Nitpick (no need to change): other similar cases in the code base still have an instanceof DagLink test if something is already a link.

vmx · 2021-02-15T13:58:57Z

src/serialize.js

 }

-exports.serializeDAGNode = serializeDAGNode
-exports.serializeDAGNodeLike = serializeDAGNodeLike
+/**


Should there be some test so that future changes already get noticed when run this libraries tests (I'm not sure how hard it is to make a proper test and whether it's worth the effort or not)?

Given the likely lifespan of this module it's probably not worth the effort.

Famous last words..

vmx · 2021-02-15T14:01:21Z

test/dag-link-test.spec.js

@@ -58,9 +59,4 @@ describe('DAGLink', () => {
 const link = new DAGLink('hello', 3, cid)
 expect(link.Hash.toBaseEncodedString()).to.equal(cid)
 })
-
- it('has an immutable CID', () => {


Is this a breaking change because it makes sense or because of breaking things while we can?

Using Object.defineProperties makes it had to infer what's a class property and what's a dynamically assigned property.

Immutability of programatic structures at this level doesn't really give us much so it seemed simpler to remove it..

vmx · 2021-02-15T14:04:50Z

test/dag-node-test.spec.js

- it('creates links from objects with .Size properties', () => {
- const node = new DAGNode(uint8ArrayFromString('some data'), [{
- Hash: 'QmUxD5gZfKzm8UN4WaguAMAZjw2TzZ2ZUmcqm2qXPtais7',
- Size: 9001
- }])
-
- expect(node.Links[0].Tsize).to.eql(9001)
- })


Shouldn't this still work (I guess with Tsize instead of Size).

It's more that Hash should be a CID but we do still handle the case. Will reinstate.

achingbrain · 2021-02-15T14:22:54Z

Some type information has no return value defined, sometimes it was even removed. What is the reason for that?

The idea is you let TSC derive the return type of a function - you can see what it'll be by mousing over the function definition or using IntelliSense or looking at the generated .d.ts file.

If you manually specify the return type, TSC will take what you specify, which sometimes is useful but more often can cause bugs because the return type of a method is not what you think it is, so it's usually better to just let the tool figure it out.

vmx · 2021-02-15T14:31:23Z

The idea is you let TSC derive the return type of a function

Make sense. Will the API docs TypeScript generates auto-fill the return values? For me (likely a minority) as API docs reader it would be unfortunate if I won't get information what the return values of a function are. Though I can totally see the point of not having return values.

achingbrain · 2021-02-17T17:35:48Z

Will the API docs TypeScript generates auto-fill the return values?

Yes, they all appear in the swanky new API docs aegir generates.

This has been rolled up to js-ipfs here: ipfs/js-ipfs#3550

vmx

Almost there.

vmx · 2021-02-18T12:14:38Z

package.json

@@ -13,7 +18,6 @@
 "release": "aegir release",
 "release-minor": "aegir release --type minor",
 "release-major": "aegir release --type major",
- "build": "aegir build",


I guess the removal of the build command is intentional?

Yes - prepare is run after installing and before publishing so there's no need for a separate build command.

package.json

vmx · 2021-02-18T12:19:11Z

src/dag-link/dagLink.js

+ this.Name = name || ''
+ this.Tsize = size
+ this.Hash = new CID(cid)


Please change it back to the original code. I don't see a reason to make a change that might break things in a subtle way if we don't have to.

vmx · 2021-02-18T12:19:34Z

src/dag-node/dagNode.js

+ this.Data = data
+ this.Links = sortedLinks
+ this._serializedSize = serializedSize
+ this._size = null


This is still an issue. I propose changing it back to the original code.

feat: add types

9d769f3

Adds types to all src and tests and fixes all tsc warnings.

chore: add types path to package.json

eb280d3

vmx reviewed Feb 11, 2021

View reviewed changes

chore: add test for identity hash

aab8f98

chore: add generated file

cc0e6ff

achingbrain marked this pull request as draft February 12, 2021 08:50

achingbrain added 2 commits February 12, 2021 13:35

chore: write out protobuf with fields in the same order as go-ipfs

16f6228

chore: remove buffer reference

5c7e050

This was referenced Feb 12, 2021

feat: add types ipfs/js-ipfs-unixfs#114

Merged

chore: upgrade deps with new typedefs ipfs/js-ipfs#3550

Merged

chore: define interface type in .d.ts to prevent duplicate types bein…

74a3727

…g generated

vmx suggested changes Feb 15, 2021

View reviewed changes

achingbrain marked this pull request as ready for review February 17, 2021 17:34

achingbrain added 2 commits February 17, 2021 18:31

chore: pr comments

517a5f1

chore: use static proto js in order to use minimal protobufjs dist

bfc59e6

vmx suggested changes Feb 18, 2021

View reviewed changes

chore: pr comments

182d0af

vmx approved these changes Feb 18, 2021

View reviewed changes

vmx merged commit 76cfef8 into master Feb 18, 2021

vmx deleted the feat/add-types branch February 18, 2021 13:43

feat: add types #189

feat: add types #189

Conversation

achingbrain commented Feb 11, 2021

codecov bot commented Feb 11, 2021 • edited Loading

Codecov Report

achingbrain commented Feb 11, 2021

vmx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achingbrain commented Feb 11, 2021

vmx commented Feb 11, 2021

achingbrain commented Feb 12, 2021

vmx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achingbrain commented Feb 15, 2021

vmx commented Feb 15, 2021

achingbrain commented Feb 17, 2021

vmx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 11, 2021 •

edited

Loading