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

Migrate to connect-es #354

Merged
merged 7 commits into from
Sep 24, 2024
Merged

Migrate to connect-es #354

merged 7 commits into from
Sep 24, 2024

Conversation

edaniels
Copy link
Member

@edaniels edaniels commented Sep 17, 2024

Hey all, we talked about doing this migration in #frontend-friends. I figured it would be best to review this code in this repo first, get it merged, and then move it over to the TS SDK. If we somehow need some patches in the meantime, we can just branch off (or worst case revert).

Connect supports pretty much everything we need (node, react native, browser, and more). We can continue the model of having a global.VIAM for setting transports and other platform specific needs.

Before:

✓ 51 modules transformed.
dist/main.js  663.78 kB │ gzip: 87.59 kB
✓ built in 678ms

After:

✓ 133 modules transformed.
dist/main.js  130.79 kB │ gzip: 31.52 kB
✓ built in 356ms

@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Sep 17, 2024
buf.lock Outdated Show resolved Hide resolved
rpc/js/tsconfig.json Outdated Show resolved Hide resolved
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Sep 17, 2024
const id = stream.getId();
const activeStream = this.streams[id];
const { id } = stream;
const activeStream = this.streams[Number(id)];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const activeStream = this.streams[Number(id)];
const activeStream = this.streams[id];

Not sure how likely we are to hit a stream count where this would matter, but maybe we should just use BigInt throughout? My understanding is that coercing between BigInt and Number could lead to precision loss per MDN docs:

Be careful coercing values back and forth, however, as the precision of a BigInt value may be lost when it is coerced to a Number value.

Copy link
Member Author

Choose a reason for hiding this comment

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

The proto changed this to a bigint so we could also just change the property on the map?

Copy link
Member

Choose a reason for hiding this comment

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

change the property on the map

yep let's do that

Copy link
Member Author

Choose a reason for hiding this comment

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

so bigint cannot be used as an index which makes sense, so how about the record key be a string and we go bigint->string on demand? it has a cost but I don't think we're in that realm of performance needs just yet

Copy link
Member

Choose a reason for hiding this comment

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

that works too

Copy link
Member

@maximpertsov maximpertsov 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!

I have one minor comment related to BigInt/Number coercion. Otherwise LGTM.

@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Sep 18, 2024
Copy link
Member

@mcous mcous left a comment

Choose a reason for hiding this comment

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

Very excited about this work, it's a huge piece of nagging tech debt off our shoulders!

I've done an initial review pass to highlight some areas of confusion for me that I'd like to focus on in our in-person review tomorrow. Also tried to highlight areas where I think extra cleanup, testing, and/or comments would be helpful, either in this PR or in the future, to ensure we're going to be able to maintain this moving forward.

If it can be practically done for this PR, we've been doing a lot of work in Fleet and Viz to use PR/commit stacking to break up large bits of work into smaller chunks. If you're down, I think splitting the updates to examples into their own PRs might make this easier to review, understand, and land. I also wonder if there's a way we could really minimize the examples changes to get a better sense of just how much of a "breaking" change this is - as opposed to opportunistic cleanup of the examples. (I think example cleanup is valuable but could happen in follow-up PRs by other contributors to gain familiarity.)

rpc/js/src/dial.ts Outdated Show resolved Hide resolved
rpc/js/src/dial.ts Outdated Show resolved Hide resolved
rpc/js/src/dial.ts Outdated Show resolved Hide resolved
rpc/examples/echo/frontend/src/index.ts Outdated Show resolved Hide resolved
rpc/examples/echo/frontend/src/index.ts Outdated Show resolved Hide resolved
rpc/js/src/dial.ts Outdated Show resolved Hide resolved
rpc/js/src/dial.ts Outdated Show resolved Hide resolved
rpc/js/src/dial.ts Outdated Show resolved Hide resolved
rpc/js/tsconfig.json Outdated Show resolved Hide resolved
rpc/js/vite.config.ts Show resolved Hide resolved
rpc/js/src/dial.ts Outdated Show resolved Hide resolved
rpc/js/src/errors.ts Outdated Show resolved Hide resolved
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Sep 20, 2024
@edaniels
Copy link
Member Author

did a bunch of cleaning. next step is a signaling refactor and a comments pass if needed. then ready for re-review. @mcous @ethanlook feel free to take a look at the ClientStream/UnaryClientStream/StreamClientStream refactor. I think it's a lot easier to read now and it minimizes the complexity of the new Promise logic as minimally as I could.

@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Sep 22, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Sep 22, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Sep 22, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Sep 22, 2024
@edaniels
Copy link
Member Author

Also made some changes to the buf yaml to reduce how many files this PR changes. There was an unneeded version upgrade.

Copy link
Member

@mcous mcous left a comment

Choose a reason for hiding this comment

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

Still working through this, but here's a quick batch of housekeeping comments just to keep things moving

rpc/js/src/BaseChannel.ts Outdated Show resolved Hide resolved
rpc/js/src/ClientStream.ts Outdated Show resolved Hide resolved
rpc/js/src/ClientChannel.ts Outdated Show resolved Hide resolved
rpc/js/src/ClientChannel.ts Outdated Show resolved Hide resolved
rpc/js/src/ClientChannel.ts Outdated Show resolved Hide resolved
rpc/js/src/ClientChannel.ts Outdated Show resolved Hide resolved
rpc/js/src/main.ts Outdated Show resolved Hide resolved
rpc/js/src/main.ts Outdated Show resolved Hide resolved
rpc/examples/echo/node-client/src/index.ts Outdated Show resolved Hide resolved
rpc/js/tsconfig.json Outdated Show resolved Hide resolved
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Sep 23, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Sep 23, 2024
Copy link
Member

@mcous mcous left a comment

Choose a reason for hiding this comment

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

I think this is a great state for a testing release! Appreciate all your work on this

rpc/js/src/dial.ts Outdated Show resolved Hide resolved
rpc/js/src/signaling-exchange.ts Show resolved Hide resolved
Copy link
Contributor

@ethanlook ethanlook left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the lint cleanup as well! And thank you again for walking us through this code.

rpc/js/src/dial.ts Outdated Show resolved Hide resolved
rpc/js/src/SignalingExchange.ts Outdated Show resolved Hide resolved
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Sep 24, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Sep 24, 2024
@edaniels edaniels merged commit 740bd5d into main Sep 24, 2024
6 of 8 checks passed
@edaniels edaniels deleted the connect-es branch September 24, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test Mark as safe to test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants