feat: ts types, github ci and clean up #56

hugomrdias · 2020-11-30T14:49:30Z

add ts types with jsdocs and aegir
remove travis and add github action
update deps and repo clean up (readme, package.json, etc.. )

- add ts types with jsdocs and aegir - remove travis and add github action - update deps and repo clean up (readme, package.json, etc.. )

vasco-santos

LGTM!

src/key.js

Co-authored-by: Vasco Santos <vasco.santos@moxy.studio>

Gozala

@hugomrdias thanks for driving this (it has been on my TODO list for quite some time). I think I'm almost happy with it, but I did offered:

Few suggestions
Type renaming

What I would however really like if we could converge onto a more granular interface(s), in fact I was adding those here https://github.com/ipfs/js-ipfs/pull/3365/files#diff-14db44021fcaf5fb4d2e74233129f8048ca4c35d372d2d470a33898944ffe449 reasoning here is that most all of the stores in ipfs-repo than can be expressed through them. If you do disagree it would be good to discuss so we can come to mutual agreement.

src/adapter.js

src/key.js

src/types.ts

Gozala · 2020-12-01T05:44:04Z

src/utils.js

-const TextEncoder = require('ipfs-utils/src/text-encoder')
-const TextDecoder = require('ipfs-utils/src/text-decoder')
+const _TextEncoder = require('ipfs-utils/src/text-encoder')
+const _TextDecoder = require('ipfs-utils/src/text-decoder')


Why _ prefix here ? If there is a reason it would be a good to have a comment explaining it.

because those aren't typed, and the name get duplicated

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>

…nto feat/ts-types * 'feat/ts-types' of github.com:ipfs/interface-datastore: Update src/key.js Update src/key.js Update src/key.js Update src/adapter.js Update src/adapter.js Update src/key.js

Gozala · 2020-12-03T17:45:08Z

What I would however really like if we could converge onto a more granular interface(s), in fact I was adding those here https://github.com/ipfs/js-ipfs/pull/3365/files#diff-14db44021fcaf5fb4d2e74233129f8048ca4c35d372d2d470a33898944ffe449 reasoning here is that most all of the stores in ipfs-repo than can be expressed through them. If you do disagree it would be good to discuss so we can come to mutual agreement.

Me and @hugomrdias had a conversation about this and I have failed to make my arguments on why interface granularity were better way to go convincing. I do not want to press this further and I’m ok with going proposed approach in this pull. That said I would like to document my arguments in favor of more granular interfaces that were my preference:

Datastore is just a more concrete key value store & it makes sense to express it the same way.
Proposed interfaces decomposed it into multiple interfaces based on their capabilities
- store lookups
- store writes
- store reads
- store imports (batch write)
- store export (batch read)
That separation of concerns enables:
- Writing functions that require subset of capabilities
- Which makes it clear to the reader what function can do (read stuff ? write stuff ? etc...)
- Type checker to ensure that no capabilities are violated
- Setup where reader and writer are different
General effect of this is e.g. I can expose / implement just read part of the datastore (e.g. over the wire one) and it will claim to implement only read & lookup interfaces. And will be compatible with all the functions that require read capabilities. Now contrast that with one interface to rule them all. It no longer possible to have over the wire store reader that works with any of the functions as they all assume read+write capabilities (even if in practice they don’t need them). In other words:
- My implementation can’t claim to satisfy existing interface
- All existing functions are incompatible because they assume read+write (at least for type checker perspective)
- And even function that don’t need write are hard to identify because types mislead.
- Instead of encouraging code that follows sepration of concerns it does the opposite.
ipfs-repo is just a just a cluster of key-value stores with diff keys and values that could be expressed by composing those smaller generic interfaces. Which in turn would enable some of the functionality be generic over key value stores regardless of keys and values. Furthermore it would encourage simple consistent API across bunch of components (that right now are slightly inconsistent).
It helpers that e.g that take simpler key value store that just implement read + write interface and return enhanced store that adds imports+exports+query regardless of the keys and values at play.

I do hope in some future we might find enough merit in the arguments above to justify decomposing datastore interface.

achingbrain

LGTM, some minor nits

achingbrain · 2020-12-15T16:23:53Z

src/adapter.js

+/**
+ * @implements {Datastore}
+ */
+class DatastoreBase {


I would either rename the file this is in, or don't rename the class. The inconsistency of having one thing called an adapter and another called a base is confusing.

achingbrain · 2020-12-15T16:44:16Z

src/key.js

+   * @param {any} value - Value to check
+   * @returns {boolean}
+   */
+  static isKey (value) {


I think we shouldn't do this sort of thing any more and just use instanceof. Boolean(value && value[symbol]) doesn't make any guarantees that what's being tested is compatible with the caller's expectation of the API.

It will break when two copies of this module are in the dep tree, but I think that's better than the returned type being incompatible.

achingbrain · 2020-12-15T16:47:42Z

src/memory.js

  async delete (key) { // eslint-disable-line require-await
    delete this.data[key.toString()]
  }

-  * _all () {
+  async * _all () {


There's no await here?

_all kinda needs to have the same return type as query

achingbrain · 2020-12-15T16:49:13Z

src/types.ts

+     * @param q
+     * @param options
+     */
+    _all(q: Query, options?: Options): AsyncIterable<Pair>;


This function isn't really part of the datastore API, it's part of the adapter implementation - we're not expecting users to call this.

Gozala · 2020-12-10T20:21:57Z

src/key.js

+   * Check if value is a Key instance
+   *
+   * @param {any} value - Value to check
+   * @returns {boolean}


Suggested change

* @returns {boolean}

* @returns {value is Key}

This would enable following to type check:

if (Key.isKey(x)) { x.child(y) }

Without this change it would not be able to refine x type to Key

Gozala

Just this needs to change https://github.com/ipfs/interface-datastore/pull/56/files?file-filters%5B%5D=.js&file-filters%5B%5D=.ts#r540470738

achingbrain

LGTM

package.json

src/adapter.js

achingbrain · 2021-01-14T13:30:02Z

src/errors.js

-module.exports.dbOpenFailedError = (err) => {
+/**
+ *
+ * @param {Error} [err]


Why aren't we documenting return types?

no specific reason, in these situations i just let TS infer from the calls. When the code is more complex and i want to make sure the return type is what i want i explicitly write the return type.

feat: ts types, github ci and clean up

2afd9be

- add ts types with jsdocs and aegir - remove travis and add github action - update deps and repo clean up (readme, package.json, etc.. )

hugomrdias requested review from achingbrain, Gozala and vasco-santos and removed request for achingbrain and Gozala November 30, 2020 14:50

vasco-santos approved these changes Nov 30, 2020

View reviewed changes

src/key.js Outdated Show resolved Hide resolved

fix: fix some types

42aebd5

hugomrdias mentioned this pull request Nov 30, 2020

feat: ts types, github ci and clean up ipfs/js-datastore-core#39

Merged

Update src/key.js

2aa294f

Co-authored-by: Vasco Santos <vasco.santos@moxy.studio>

hugomrdias mentioned this pull request Nov 30, 2020

Track: Typescript types support ipfs/js-ipfs#2945

Closed

Gozala suggested changes Dec 1, 2020

View reviewed changes

hugomrdias and others added 7 commits December 2, 2020 14:26

Update src/adapter.js

8e8216d

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>

Update src/adapter.js

56d12c6

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>

Update src/key.js

e6f03da

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>

Update src/key.js

fe1aa1b

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>

Update src/key.js

7a4d1d1

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>

Merge branch 'feat/ts-types' of github.com:ipfs/interface-datastore i…

0b8a1f8

…nto feat/ts-types * 'feat/ts-types' of github.com:ipfs/interface-datastore: Update src/key.js Update src/key.js Update src/key.js Update src/adapter.js Update src/adapter.js Update src/key.js

fix: feedback

248cddb

hugomrdias requested a review from Gozala December 3, 2020 14:38

hugomrdias added 3 commits December 3, 2020 16:15

chore: change build to prepare

63c1a82

chore: change prepare to prepack

5b8f8ae

chore: change prepare

75e315b

chore: fix ci

29f094a

This was referenced Dec 9, 2020

chore: add typedefs libp2p/js-libp2p#802

Merged

Followup: Types libp2p/js-libp2p#830

Closed

fix: types

f8fe99e

hugomrdias added 4 commits December 11, 2020 11:00

fix: ci

f197aa4

ci

98607c0

prepack

5ea6e72

prepare

e425f20

This was referenced Dec 11, 2020

feat: types ipfs/js-datastore-fs#62

Merged

feat: types ipfs/js-datastore-level#53

Merged

achingbrain linked an issue Dec 15, 2020 that may be closed by this pull request

Additional types maintainer(s)? #44

Closed

achingbrain suggested changes Dec 15, 2020

View reviewed changes

Gozala approved these changes Dec 17, 2020

View reviewed changes

Gozala suggested changes Dec 17, 2020

View reviewed changes

fix: update aegir and feedback

eab84b0

hugomrdias requested a review from achingbrain January 11, 2021 16:12

fix: remove types versions and tweak orders

e449528

achingbrain reviewed Jan 14, 2021

View reviewed changes

chore: remove gh url

90b6ab6

achingbrain merged commit 8ea7888 into master Jan 15, 2021

achingbrain deleted the feat/ts-types branch January 15, 2021 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ts types, github ci and clean up #56

feat: ts types, github ci and clean up #56

hugomrdias commented Nov 30, 2020

vasco-santos left a comment

Gozala left a comment

Gozala Dec 1, 2020

hugomrdias Dec 2, 2020

Gozala commented Dec 3, 2020

achingbrain left a comment

achingbrain Dec 15, 2020

achingbrain Dec 15, 2020

achingbrain Dec 15, 2020

hugomrdias Dec 17, 2020

achingbrain Dec 15, 2020

Gozala Dec 10, 2020

Gozala left a comment

achingbrain left a comment

achingbrain Jan 14, 2021

hugomrdias Jan 14, 2021

feat: ts types, github ci and clean up #56

feat: ts types, github ci and clean up #56

Conversation

hugomrdias commented Nov 30, 2020

vasco-santos left a comment

Choose a reason for hiding this comment

Gozala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gozala commented Dec 3, 2020

achingbrain 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

Gozala left a comment

Choose a reason for hiding this comment

achingbrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment