-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
Updates deps and adds types to tests. BREAKING CHANGE: needs ESM release of ipfs/libp2p
CI won't pass until new js-ipfs ships. |
go1.api.swarm.connect(go0.api.peerId.addresses[0]), | ||
js0.api.swarm.connect(go0.api.peerId.addresses[0]), | ||
go0.api.swarm.connect(js0.api.peerId.addresses[0]) | ||
js0.api.swarm.connect(js1.peer.addresses[0]), |
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.
Some type failures here. not sure if they're expected.
@@ -5,11 +5,16 @@ import allV1 from './circuit/v1/all.js' | |||
import allV2 from './circuit/v2/all.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.
You should add the ts-check
pragma to the top of files if you're using jsdoc typings until you move this to typescript.
@@ -94,13 +101,15 @@ const timeout = isCi ? 15 * min : 10 * min | |||
describe('exchange files', function () { | |||
this.timeout(timeout) | |||
|
|||
/** @type {Record<string, ('go' | '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.
This seems to be an incorrect type for what you're trying to accomplish. lmk if you want to allow a 3-tuple.
type arr = ('go' | 'js')[]
const array: arr = ['go', 'js', 'go'] // False Positive
const array1: arr = ['go', 'go'] // True positive
const array2: arr = ['go', 'js'] // True positive
const array3: arr = ['js', 'go'] // True positive
const array4: arr = ['js', 'js'] // True positive
type goOrJs = 'go' | 'js'
type arrFix = [goOrJs, goOrJs]
const arrayFixed: arrFix = ['go', 'js', 'go'] // True Negative
const arrayFixedException1: arrFix = ['go', 'go'] // True positive
const arrayFixedException2: arrFix = ['go', 'js'] // True positive
const arrayFixedException3: arrFix = ['js', 'go'] // True positive
const arrayFixedException4: arrFix = ['js', 'js'] // True positive
Playground Link: Provided
@@ -50,6 +60,11 @@ describe('pin', function () { | |||
return daemon | |||
} | |||
|
|||
/** | |||
* @template T |
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.
You should restrict the types of your template. @template {Type} T
// @ts-expect-error env var could be undefined | ||
ipfsHttpModule = await import(process.env.IPFS_JS_HTTP_MODULE) | ||
} catch { | ||
ipfsHttpModule = await import('ipfs-http-client') |
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.
Elsewhere you use await import(process.env.IPFS_JS_HTTP_MODULE || 'ipfs-http-client')
should we create a wrapper for the module import and use that everywhere?
// @ts-expect-error env var could be undefined | ||
ipfsModule = await import(process.env.IPFS_JS_MODULE) | ||
} catch { | ||
ipfsModule = await import('ipfs') |
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.
ditto
// @ts-expect-error no types | ||
import goenv from 'go-platform' |
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.
create global.d.ts with declare module 'go-platform'
test/utils/relayd.js
Outdated
@@ -28,7 +37,7 @@ export async function getRelayV (version, factory) { | |||
id = text.split('I am')[1].split('\n')[0].trim() | |||
} | |||
} | |||
const config = JSON.parse(fs.readFileSync(configPath)) | |||
const config = JSON.parse(fs.readFileSync(configPath).toString()) |
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.
try/catch around this?
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 (the changes since last review)
tried running code from this locally since I'm on mac and that CI test is failing and I get a different error:
|
here is the output from top of the stack trace:
|
That's very strange - I'm also on a Mac and I don't see any failures at all, nor on Windows running under Parallels, only on CI. *shakes fist at sky*. I didn't have any time to look at this more today, I'll try again tomorrow. |
Yes, M1 Max and node 16.17.0. The only difference I can see is that the CI machine will be resource constrained and my laptop significantly less so, so there must be a difference in timing somewhere. |
I just saw this locally running the tests on a linux VM. Removing the downloaded libp2p-relay-daemon binary and old |
Updates all dependencies and adds types to tests. BREAKING CHANGE: uses ESM release of ipfs/libp2p
Wow @achingbrain ! You're a hero. Thanks a lot! |
Updates all dependencies and adds types to tests. BREAKING CHANGE: uses ESM release of ipfs/libp2p
Updates all dependencies and adds types to tests. BREAKING CHANGE: uses ESM release of ipfs/libp2p
@achingbrain you rock! |
Updates deps and adds types to tests.
BREAKING CHANGE: needs ESM release of ipfs/libp2p