Skip to content

Feat: Client validate method#1340

Merged
dOrgJelli merged 21 commits into
origin-devfrom
feat/client-validate
Dec 3, 2022
Merged

Feat: Client validate method#1340
dOrgJelli merged 21 commits into
origin-devfrom
feat/client-validate

Conversation

@cbrzn
Copy link
Copy Markdown
Contributor

@cbrzn cbrzn commented Oct 17, 2022

This PR aims to add the validate function to the client, being able to guarantee that the client can communicate with the given wrapper URI ahead-of-time

The interface is the following:

interface ValidateOptions {
  recursive: boolean
  abi: boolean
}
validate(uri: Uri | string, options: ValidateOptions) -> boolean
  • If abi is true, make sure the ABI from 1st level dependencies are available (the ones from importedModuleType)
  • If recursive is true, it will check all uris and make sure it can resolve them

Also, a compare function has been implemented in js/manifests/wrap to compare two array of method definitions, this function allow us to make sure that dependencies are equal and wont fail in runtime

Copy link
Copy Markdown
Contributor

@krisbitney krisbitney left a comment

Choose a reason for hiding this comment

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

This is amazing! I think this will be an extremely useful addition to the client. Very awesome work.

I made some suggestions for possible changes, and I highlighted a couple of minor mistakes.

A feature that could be nice (likely for a future PR) would be the ability to ignore validation of some URIs. To me, it's really convenient that it is possible to use a wrapper without all of its dependencies. A dependency might only be necessary for some methods that aren't being used. So it would be nice to have a way to incorporate that sort of information into the validation check.

I want to reiterate once again how amazing this is. The options you included here are really fantastic. 🐐🐐🐐

Comment thread packages/js/client/src/PolywrapClient.ts Outdated
Comment thread packages/js/client/src/PolywrapClient.ts Outdated
Comment thread packages/js/client/src/PolywrapClient.ts Outdated
Comment thread packages/js/client/src/PolywrapClient.ts Outdated
Comment thread packages/js/client/src/__tests__/core/sanity.spec.ts Outdated
Comment thread packages/js/client/src/__tests__/core/sanity.spec.ts Outdated
Comment thread packages/js/core/src/types/Client.ts Outdated
Comment thread packages/js/manifests/wrap/scripts/templates/compare-ts.mustache Outdated
Comment thread packages/js/manifests/wrap/src/formats/wrap.info/compare.ts Outdated
@dOrgJelli dOrgJelli linked an issue Oct 18, 2022 that may be closed by this pull request
@cbrzn cbrzn requested a review from krisbitney October 18, 2022 17:57
krisbitney
krisbitney previously approved these changes Oct 19, 2022
Copy link
Copy Markdown
Contributor

@krisbitney krisbitney left a comment

Choose a reason for hiding this comment

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

This is such a cool feature!

@dOrgJelli dOrgJelli changed the base branch from origin-dev to origin-0.10-dev October 20, 2022 20:46
@dOrgJelli dOrgJelli changed the base branch from origin-0.10-dev to origin-dev November 15, 2022 19:29
Niraj-Kamdar
Niraj-Kamdar previously approved these changes Nov 17, 2022
Copy link
Copy Markdown
Contributor

@Niraj-Kamdar Niraj-Kamdar left a comment

Choose a reason for hiding this comment

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

This is awesome!

Copy link
Copy Markdown
Contributor

@pileks pileks left a comment

Choose a reason for hiding this comment

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

compare.ts is exporting a single function. Would it make sense to rename it to compareSignatures.ts, additionally export default the func?
Maybe also move it into a utils folder?

I'm thinking about this mainly from a ease-of-use, ease-of-finding point of view.

Comment thread packages/js/manifests/wrap/src/compare.ts Outdated
@cbrzn cbrzn requested review from krisbitney and pileks November 28, 2022 16:22
@cbrzn cbrzn force-pushed the feat/client-validate branch from 801e6a0 to cb24354 Compare November 28, 2022 17:22
@cbrzn cbrzn requested a review from Niraj-Kamdar November 29, 2022 14:32
@dOrgJelli dOrgJelli merged commit bb01d39 into origin-dev Dec 3, 2022
@dOrgJelli dOrgJelli deleted the feat/client-validate branch April 10, 2023 17:01
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.

client.validate(uri) - Validate Wrapper->Client Compatibility

5 participants