Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

chore: upgrade deps with new typedefs #3550

Merged
merged 12 commits into from
Mar 31, 2021
Merged

chore: upgrade deps with new typedefs #3550

merged 12 commits into from
Mar 31, 2021

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Feb 15, 2021

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

Depends on:

To do:

  • Entype core-api in ipfs-core-types package
  • Make ipfs-core implement types from ipfs-core-types package
  • Make ipfs-grpc-* implement types from ipfs-core-types package
  • Make ipfs-message-port-* implement types from ipfs-core-types package
  • Make ipfs-http-client implement types from ipfs-core-types package
  • Make ipfs-http-server implement types from ipfs-core-types package
  • Make ipfs-http-gateway implement types from ipfs-core-types package
  • Make ipfs-daemon implement types from ipfs-core-types package
  • Make ipfs-cli implement types from ipfs-core-types package
  • Make ipfs implement types from ipfs-core-types package
  • Resolve main/exports esm/cjs aegir/ipjs imports
  • Fix tests

@achingbrain achingbrain changed the title chore: upgrade unixfs deps chore: upgrade deps with new typedefs Mar 5, 2021
@@ -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
Copy link
Member

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(),
Copy link
Member

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) {
Copy link
Member

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?

Copy link
Member

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), {
Copy link
Member

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
Copy link
Member

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?

@achingbrain achingbrain force-pushed the chore/update-unixfs branch from 8d63c7c to e443160 Compare March 13, 2021 14:53
@BigLep BigLep added the status/in-progress In progress label Mar 15, 2021
achingbrain added a commit to ipfs/js-ipfsd-ctl that referenced this pull request Mar 21, 2021
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
@achingbrain achingbrain linked an issue Mar 21, 2021 that may be closed by this pull request
Copy link
Member

@hugomrdias hugomrdias left a 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.

packages/ipfs-cli/src/commands/add.js Outdated Show resolved Hide resolved
packages/ipfs-cli/src/commands/bitswap/stat.js Outdated Show resolved Hide resolved
packages/ipfs-cli/src/commands/bitswap/unwant.js Outdated Show resolved Hide resolved
packages/ipfs-cli/src/commands/bitswap/wantlist.js Outdated Show resolved Hide resolved
packages/ipfs-cli/src/commands/block/put.js Outdated Show resolved Hide resolved
@@ -0,0 +1,136 @@
import CID from 'cids'
import { AwaitIterable } from './basic'
Copy link
Member

Choose a reason for hiding this comment

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

is this importing itself ?

Copy link
Member Author

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>
Copy link
Member

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 ?

Copy link
Member Author

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?

Copy link
Member

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

packages/ipfs-core-types/src/block/index.d.ts Show resolved Hide resolved
packages/ipfs-core-types/src/block/index.d.ts Show resolved Hide resolved
packages/ipfs-core-types/src/files/index.d.ts Show resolved Hide resolved
achingbrain added a commit to ipfs/js-ipfsd-ctl that referenced this pull request Mar 26, 2021
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
@achingbrain achingbrain force-pushed the chore/update-unixfs branch from 3f94436 to df7e158 Compare March 27, 2021 15:10
@achingbrain achingbrain merged commit a418a52 into master Mar 31, 2021
@achingbrain achingbrain deleted the chore/update-unixfs branch March 31, 2021 13:49
@achingbrain achingbrain removed the status/in-progress In progress label Apr 1, 2021
oliveriosousa pushed a commit to oliveriosousa/js-ipfs that referenced this pull request Jul 26, 2021
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
SgtPooki referenced this pull request in ipfs/js-kubo-rpc-client Aug 18, 2022
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Will not build on Windows 10 64-bit
4 participants