-
Notifications
You must be signed in to change notification settings - Fork 1.2k
chore: upgrade deps with new typedefs #3550
Conversation
@@ -210,13 +210,19 @@ module.exports = (context) => { | |||
dagBuilder: async function * (source, block, opts) { | |||
for await (const entry of source) { | |||
yield async function () { | |||
const cid = await persist(entry.content.serialize(), block, opts) | |||
/** @type {DAGNode} */ | |||
// @ts-ignore |
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.
I think you get to remove this ignore now
size: size, | ||
cumulativeSize: cumulativeSize, | ||
blocks: blocks, | ||
size: file.unixfs.fileSize(), |
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.
do we have a guarantee now that there is a file.unixfs
?
} | ||
|
||
if (file.unixfs) { |
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.
the things I think are missing from this deleted block now are - stat.type
setting (it's always 'file'
now), stat.size
set to 0
for isDirectory()
, should these be added back?
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 this is now safely disambiguating the file vs directory case before we even get here ... I guess that makes them unnecessary here!
}, bucket, pos)) | ||
|
||
return Promise.resolve() | ||
} | ||
|
||
return (rootBucket || bucket).put(link.Name.substring(2), { | ||
return rootBucket.put(link.Name.substring(2), { |
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.
I'm not seeing the guarantee in the changes here that there will be a rootBucket
at this point. It's added in recreateHamtLevel
if there's not a parentBucket
, but are we sure that there's always a rootBucket
if there's a parentBucket
? A quick git grep
suggests that recreateHamtLevel
either has only the first arg or all of the args, but I don't know whether those args always have a non-null value.
@@ -127,7 +127,7 @@ const updateShard = async (context, positions, child, options) => { | |||
|
|||
log(`Updating shard ${prefix} with name ${newName}`) | |||
|
|||
const size = DAGNode.isDAGNode(result.node) ? result.node.size : result.node.Tsize | |||
const size = result.node instanceof DAGNode ? result.node.size : result.node.Tsize |
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.
I always get told off by browser people for using instanceof
, isn't this something we're supposed to avoid by duck typing at a minimum?
8d63c7c
to
e443160
Compare
ipfs/js-ipfs#3550 switches away from default exports for `ipfs-http-client` and `ipfs-client` to enable an easier transition to es6 modules. All `ipfs` modules now export a `.create` factory function which returns instances of the client module. BREAKING CHANGE: expects `ipfs-http-client` and `ipfs-client` to export a `.create` function
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.
partial review
some comments inline, im a little worried that our inputs are forcing the users to pull in CID
, PeerId
, multiaddr
, ipld-block
etc etc
i know we talked about this a million times and i understand why its like this but i dunno if its the right way.
So if we force the input to be the instance of those classes we should do it everywhere, and put an @see 'url to package' in the js docs.
Or we can accept the low level version of those like string for CID or UInt8array for Block etc.
@@ -0,0 +1,136 @@ | |||
import CID from 'cids' | |||
import { AwaitIterable } from './basic' |
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.
is this importing itself ?
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.
Oops, looks like it.
*/ | ||
export type ToContent = | ||
| string | ||
| InstanceType<typeof String> |
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.
this can't be just string
?
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.
If we want to support 'string'
and new String('string')
then no as they are different.
Who does that though?
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.
lol right, this is just a nit so feel free to ignore
ipfs/js-ipfs#3550 switches away from default exports for `ipfs-http-client` and `ipfs-client` to enable an easier transition to es6 modules. All `ipfs` modules now export a `.create` factory function which returns instances of the client module. BREAKING CHANGE: expects `ipfs-http-client` and `ipfs-client` to export a `.create` function
- Uses default aegir ts config - Fixes all ts errors - Fully types core-api in ipfs-core-types package - Makes ipfs-core implement types from ipfs-core-types package - Removes duplicate types, ipfs-core-types as single source of type truth - Reduces use of external APIs by internal components in ipfs-core
3f94436
to
df7e158
Compare
Upgrades to new version with types - Uses default aegir ts config - Fixes all ts errors - Fully types core-api in ipfs-core-types package - Makes ipfs-core implement types from ipfs-core-types package - Removes duplicate types, ipfs-core-types as single source of type truth - Reduces use of external APIs by internal components in ipfs-core - Switches to named exports BREAKING CHANGE: all core api methods now have types, some method signatures have changed, named exports are now used by the http, grpc and ipfs client modules
Upgrades to new version with types - Uses default aegir ts config - Fixes all ts errors - Fully types core-api in ipfs-core-types package - Makes ipfs-core implement types from ipfs-core-types package - Removes duplicate types, ipfs-core-types as single source of type truth - Reduces use of external APIs by internal components in ipfs-core - Switches to named exports BREAKING CHANGE: all core api methods now have types, some method signatures have changed, named exports are now used by the http, grpc and ipfs client modules
Upgrades to new version with types
Depends on:
.create
function for http client and ipfs client js-ipfsd-ctl#616To do:
ipfs-core-types
packageipfs-core
implement types fromipfs-core-types
packageipfs-grpc-*
implement types fromipfs-core-types
packageipfs-message-port-*
implement types fromipfs-core-types
packageipfs-http-client
implement types fromipfs-core-types
packageipfs-http-server
implement types fromipfs-core-types
packageipfs-http-gateway
implement types fromipfs-core-types
packageipfs-daemon
implement types fromipfs-core-types
packageipfs-cli
implement types fromipfs-core-types
packageipfs
implement types fromipfs-core-types
package