-
Notifications
You must be signed in to change notification settings - Fork 1
feat(sdk): connect rpc client export #545
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
base: main
Are you sure you want to change the base?
Conversation
If these changes look good, signoff on them with:
If they aren't any good, please remove them with:
|
@@ -72,13 +77,17 @@ | |||
"watch": "(trap 'kill 0' SIGINT; npm run build && (npm run build:watch & npm run test -- --watch))" | |||
}, | |||
"dependencies": { | |||
"@connectrpc/connect": "^2.0.2", |
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.
These have been added as dependencies and are re-exported in lib/src/platform.ts
. This way we can ensure that the generated client services will work with the connectrpc web framework.
One alternative I've considered here is to declare them as peerDependencies
and actually not re-export them. When someone consumes @opentdf/sdk
they will get a warning that @connectrcp
needs to be added to their package.json
.
I would have pushed for this option if the bundle size increased, but it has not. I think we are good with re-export
🤖 🎨 Autoformat
🤖 🎨 Autoformat
ab1257d
to
0f028e0
Compare
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.
Overall, this seems like a good consolidation over the previous work.
Have you actually managed to make these work? If you want to test them, you'll probably want to add code to the integration test packages (in the cli and web-app folders).
I've tested the wellknownconfig endpoint with the local build to make sure it was exported and functional. (Thats how I got the example for the PR description) As a follow up I do have to update the @dmihalcik-virtru |
Given that this is a refactor of existing code, I'm okay approving without a working integration test. However, it does actually expose the feature (I think? how do you initialize the client?), so we should get a test working before the next release to NPM. For me, I'd like some code that shows how it works, something the integration test would perforce include |
oh sorry I see now in the PR description. We maybe still want to add it to the readme (maybe after doing the integration test), and I think the sample code in the description still needs auth configured |
If these changes look good, signoff on them with:
If they aren't any good, please remove them with:
|
23716d8
to
c007567
Compare
|
Description
Generates and exports connectrpc service definitions.
PR Changes
generate-platform
to run the generatorweb-app
Consumer Usage Example
This is an example of how the generated client can be used:
Generating
To generate the service definition run:
This will clone the
platform
into the repository and execute the generators based on the exported protos in the platform servicesDetails
To allow proto buf genrators to work, the platform needs to be cloned into
lib/src/platform
diretory. The scriptscripts/platform.sh
handles that.lib/buf.gen.yaml
is the configuration for the generator. To execute the generator manuallycd lib npx buf generate ../platform/service
This will generate
lib/src/platform
exports for each of the service.Finally,
lib/src/platform.ts
re-exports those services and the connect rpc frameworkFollow up