Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: w3up-client tsconfig.json now includes "./test" so its tests have some type checking #669

Merged
merged 6 commits into from
Mar 29, 2023

Conversation

gobengo
Copy link
Contributor

@gobengo gobengo commented Mar 28, 2023

Non-goal

  • changing src code
  • avoidance of all ts-ignore in these newly included files (that can come as iteration)

Context:

@gobengo gobengo requested a review from travis March 28, 2023 23:22
@gobengo gobengo marked this pull request as ready for review March 28, 2023 23:30
@@ -124,17 +124,19 @@ export class Client extends Base {
/**
* Use a specific space.
*
* @param {import('./types').DID<'key'>} did
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverting this ts api interface change web3-storage/w3up-client@ee1cf3a#r106486919

(let's introduce the DID -> DID<'key'> again in one fell swoop that also adjusts some @web3-storage/access/agent-data types that still need to be done)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops this was not 'releasable' due to using test: conventional commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change went out in [email protected]

@@ -33,6 +33,7 @@ describe('AccessClient', () => {
})

const alice = new Client(await AgentData.create(), {
// @ts-ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may be able to get rid of these ts-ignore (remember: the whole file was ignored by tsc before) in a followup PR by removing the voucher type prop from @web3-storage/access/types ServiceType.

@gobengo gobengo requested a review from alanshaw March 28, 2023 23:34
Copy link
Member

@travis travis left a comment

Choose a reason for hiding this comment

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

this looks good to me! I'd definitely feel a bit comfier if @alanshaw or @Gozala weighed in since some of this stuff makes changes to capabilities that don't intuitively make sense to me, but it's tests and seems like an improvement over not typechecking this stuff, so +1

@gobengo
Copy link
Contributor Author

gobengo commented Mar 28, 2023

since some of this stuff makes changes to capabilities that don't intuitively make sense to me

@travis it makes changes to some w3up-client tests that have the word 'capability' in their file name, but notably it does not make any changes to the @web3-storage/capabilities package, i.e. the canonical capability parsers that sorta define the protocol.

with that said, no worries at all waiting for a review from @alanshaw or @Gozala as well

@gobengo gobengo requested a review from Gozala March 29, 2023 00:00
@@ -51,9 +47,6 @@ describe('SpaceClient', () => {
assert.equal(service.space.info.callCount, 1)

assert.equal(info.did, space.did())
assert.equal(info.agent, alice.agent().did())
assert.equal(info.email, 'mailto:[email protected]')
assert.equal(info.product, 'product:test')
Copy link
Member

Choose a reason for hiding this comment

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

Why these removals?

Copy link
Contributor Author

@gobengo gobengo Mar 29, 2023

Choose a reason for hiding this comment

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

in order to make the type checker pass, I needed to either

  • add a couple fields that I didn't understand
    • I tried, eventually it was mad I wasn't about certain fields.
    • That type is kinda weird because it was authored derived from the kysely ORM db row type vs something more abstract
    • Which is why in spaces 'registered' via the newer provider/add, the type for space/info success is much narrower and only { did: DID<'key'> } right now. (no tests require more than that, but I suspect some will)
  • or just return the new narrower { did: DID<'key'> } here to make this pass tsc and generally be a realistic mock of a space info result

@gobengo gobengo merged commit f0883c3 into main Mar 29, 2023
@gobengo gobengo deleted the 668-w3up-client-tsc-test branch March 29, 2023 15:16
gobengo added a commit that referenced this pull request Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants