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

feat!: Use schema stuff in the capabilities instead of custom parsing #220

Merged
merged 8 commits into from
Feb 14, 2023

Conversation

Gozala
Copy link
Collaborator

@Gozala Gozala commented Feb 9, 2023

capability logic came before the Schema did we have not got around to integrating schema fully into capability we even end up with bunch of code that duplicated logic that is in schema.

Consequently it became impossible to use Schema.struct or Schema.dict on the nb field. This PR updates capability to just use Schema.

@Gozala Gozala changed the title feat: .partial for schema with map representation feat!: Use schema stuff in the capabilities instead of custom parsing Feb 10, 2023
@Gozala Gozala requested a review from gobengo February 10, 2023 01:20
}) => new Capability({ derives, nb, ...etc })

const defaultNBSchema =
/** @type {Schema.MapRepresentation<any>} */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** @type {Schema.MapRepresentation<any>} */
/** @type {Schema.MapRepresentation<unknown>} */

not sure if this makes sense but just an idea..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No this is not going to work I'm afraid because it's used as default parameter for generic object schema and anything but any would fail to meet that requirement.

Ideally TS would respect default type parameter and allow matching it with default parameter in the function, but sadly it doesn't work so this is the only workaround I'm aware of

@@ -1503,10 +1506,10 @@ test('capability with optional caveats', async () => {
const Echo = capability({
can: 'test/echo',
with: URI.match({ protocol: 'did:' }),
nb: {
nb: Schema.struct({
Copy link
Contributor

Choose a reason for hiding this comment

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

wdyt of trying to make things backward incompatible so code like nb: { message: URI } still works even without wrapping with Schema.struct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to, but amount of added complexity coupled with the fact that TS inference gets confused on occasions I don’t think it would be worth it

@Gozala Gozala merged commit 8a578ae into main Feb 14, 2023
@github-actions github-actions bot mentioned this pull request Feb 14, 2023
@github-actions github-actions bot mentioned this pull request Feb 28, 2023
This was referenced 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.

2 participants