Skip to content

Migrate SDK to connect-es #368

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

Merged
merged 8 commits into from
Oct 4, 2024
Merged

Conversation

edaniels
Copy link
Contributor

@edaniels edaniels commented Sep 27, 2024

I need to check some integrations with app but this ready to review.

Major Changes

  • connect-es is in use now which breaks every usage of proto. e.g. pbObj.getSomeField() is now pbObj.someField
  • AsObject gets removed everywhere
  • I unified Credentials into a single type since it had two definitions across rpc/js and the sdk
    • Someone will need to update app examples. Alternatively, I can change the type back to have authEntity be separate. Someone let me know!
  • React Native has many more polyfills in order to satisfy the modern nature of connect-es, sadly. We could make this simpler in the future probably.

Minor Fixes

  • exponential-backoff now works as expected. It was a classic setting a property to undefined makes it present/set.

This PR is unfortunately huge but everything works and is tested across every example. Happy to go over anything in person though or if there's a better way to split it up, just holler. But... it's more deletions than additions and everything should be easier to read.

@edaniels edaniels requested a review from njooma September 27, 2024 20:52
@edaniels edaniels requested a review from a team as a code owner September 27, 2024 20:52
@edaniels edaniels requested a review from lia-viam September 27, 2024 20:52
:license_links: https://www.npmjs.com/package/@improbable-eng/grpc-web-fake-transport
:versions: []
:when: 2023-05-01 18:11:37.804857463 Z
- - :approve
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need approval for this addition. It is apache 2 and bsd-3

@@ -31,7 +31,7 @@ test-watch: $(node_modules) build-buf
lint: $(node_modules) build-buf
npm run lint
npm run typecheck
npm run check
npm run check -- --ignore=@bufbuild/protobuf
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the new major version (2) is incompatible with other packages used by connect-es

Copy link
Member

@njooma njooma left a comment

Choose a reason for hiding this comment

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

Not going through every line of code but did some tests and it all works so 👍

@edaniels edaniels merged commit e1f3569 into viamrobotics:main Oct 4, 2024
3 checks passed
@edaniels edaniels deleted the connect-es branch October 4, 2024 19:17
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