-
Notifications
You must be signed in to change notification settings - Fork 12
Conversation
hugomrdias
commented
Nov 30, 2020
•
edited
Loading
edited
- 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.. )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly good. Just minor things to be fixed
has (key, options) { | ||
const match = this._lookup(key) | ||
if (match == null) { | ||
return false | ||
return Promise.resolve(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return types are inferred from the interface. However, it would probably be helpful to have them in the code, special for returning promises. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe interface should be changed to has(key:Key, options?:Options): Await<boolean>
instead so wrapping in promise is unnecessary ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gozala for that we would need to make it an async function
@vasco-santos i would like to avoid writing all the return types if they are already well inferred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gozala for that we would need to make it an async function
I don't believe that's accurate, in fact that was the whole point of Await
type. Here is an example:
type Await<T> = Promise<T> | T
interface Key {
toString():string
}
interface Has {
has(key:Key, options?:{signal?: AbortSignal}):Await<boolean>
}
class Store implements Has {
has(key:Key, options:{signal?: AbortSignal}):Await<boolean> {
if (key.toString().startsWith('http://')) {
return fetch(key.toString()).then(response => response.ok)
} else {
return false
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a pattern I think Await<T>
is something we should allow. The idea of the return type being variable is a bit weird, but in the past we've desired the ability to not create artificial asynchronicity for super-hot codepaths, the idea being you'd look at the return type and resolve the promise if it's a thenable.
Though it's not something we should be doing everywhere, blindly await
ing will be fine 99.9% of the time, unless some rigorous profiling has identified a bottleneck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does make the error handling for the caller a bit Zalgo harder to handle, mind:
async function derp (input) {
if (input % 2 === 1) {
throw new Error('Input was odd')
}
return Promise.reject(new Error('Input was even'))
}
const res1 = derp(1) // throws immediately
const res2 = derp(2)
// maybe do some other stuff
await res2 // throws here instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think we should do here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Mostly provided some minor suggestions, I do however would like type-check succeed here instead of failing due to lack of generated types.
Co-authored-by: Vasco Santos <[email protected]>
Co-authored-by: Vasco Santos <[email protected]>
Co-authored-by: Vasco Santos <[email protected]>
Co-authored-by: Vasco Santos <[email protected]>
Co-authored-by: Vasco Santos <[email protected]>
Co-authored-by: Vasco Santos <[email protected]>
Co-authored-by: Vasco Santos <[email protected]>
Co-authored-by: Vasco Santos <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
…core into feat/ts-types * 'feat/ts-types' of https://github.com/ipfs/js-datastore-core: (24 commits) Update src/tiered.js Update src/sharding.js Update src/sharding.js Update src/sharding.js Update src/sharding.js Update src/sharding.js Update src/sharding.js Update src/sharding.js Update src/tiered.js Update src/tiered.js Update src/tiered.js Update src/keytransform.js Update src/keytransform.js Update src/keytransform.js Update src/keytransform.js Update .github/workflows/main.yml Update src/shard.js Update src/namespace.js Update src/namespace.js Update src/mount.js ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recent changes seem to have updated all the import paths from interface-datastore/src/*
to interface-datastore/dist/src/*
. I do not know if that is deliberate, but I do think it's great as it is going to be confusing.