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

Enable strict type checking #303

Merged
merged 10 commits into from
Oct 21, 2024
Merged

Enable strict type checking #303

merged 10 commits into from
Oct 21, 2024

Conversation

lukasIO
Copy link
Contributor

@lukasIO lukasIO commented Oct 16, 2024

This also fixes all errors that showed up and paves the way for more ergonomic usage by upgrading protobuf-es to v2.x (once the protovalidate types are available )

Copy link

changeset-bot bot commented Oct 16, 2024

🦋 Changeset detected

Latest commit: ebcfafc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@livekit/rtc-node Patch
@livekit/rtc-node-darwin-arm64 Patch
@livekit/rtc-node-darwin-x64 Patch
@livekit/rtc-node-linux-arm64-gnu Patch
@livekit/rtc-node-linux-x64-gnu Patch
@livekit/rtc-node-win32-x64-msvc Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@lukasIO lukasIO merged commit f204721 into main Oct 21, 2024
10 checks passed
@lukasIO lukasIO deleted the lukas/strict-ts branch October 21, 2024 19:28
@github-actions github-actions bot mentioned this pull request Oct 21, 2024
@bcherry
Copy link
Contributor

bcherry commented Oct 23, 2024

@lukasIO does this actually work for you? There aren't any automated tests on livekit-rtc, so the green check is misleading, but all of the examples fail due to issues with missing required fields at serialization as described in this issue (that I see you're already active in!)

I think we might need to roll this back...

file:///Users/ben/dev/livekit/node-sdks/node_modules/.pnpm/@[email protected]/node_modules/@bufbuild/protobuf/dist/esm/to-binary.js:34
                throw new Error(`cannot encode field ${msg.desc.typeName}.${f.name} to binary: required field not set`);
                      ^

Error: cannot encode field livekit.proto.RoomOptions.adaptive_stream to binary: required field not set
    at writeFields (file:///Users/ben/dev/livekit/node-sdks/node_modules/.pnpm/@[email protected]/node_modules/@bufbuild/protobuf/dist/esm/to-binary.js:34:23)
    at writeMessageField (file:///Users/ben/dev/livekit/node-sdks/node_modules/.pnpm/@[email protected]/node_modules/@bufbuild/protobuf/dist/esm/to-binary.js:78:9)
    at writeField (file:///Users/ben/dev/livekit/node-sdks/node_modules/.pnpm/@[email protected]/node_modules/@bufbuild/protobuf/dist/esm/to-binary.js:61:13)
    at writeFields (file:///Users/ben/dev/livekit/node-sdks/node_modules/.pnpm/@[email protected]/node_modules/@bufbuild/protobuf/dist/esm/to-binary.js:38:9)
    at writeMessageField (file:///Users/ben/dev/livekit/node-sdks/node_modules/.pnpm/@[email protected]/node_modules/@bufbuild/protobuf/dist/esm/to-binary.js:78:9)
    at writeField (file:///Users/ben/dev/livekit/node-sdks/node_modules/.pnpm/@[email protected]/node_modules/@bufbuild/protobuf/dist/esm/to-binary.js:61:13)
    at writeFields (file:///Users/ben/dev/livekit/node-sdks/node_modules/.pnpm/@[email protected]/node_modules/@bufbuild/protobuf/dist/esm/to-binary.js:38:9)
    at toBinary (file:///Users/ben/dev/livekit/node-sdks/node_modules/.pnpm/@[email protected]/node_modules/@bufbuild/protobuf/dist/esm/to-binary.js:27:12)
    at FfiClient.request (/Users/ben/dev/livekit/node-sdks/packages/livekit-rtc/src/ffi_client.ts:56:22)
    at Room.connect (/Users/ben/dev/livekit/node-sdks/packages/livekit-rtc/src/room.ts:133:36)

Node.js v20.16.0

lukasIO added a commit that referenced this pull request Oct 23, 2024
@lukasIO
Copy link
Contributor Author

lukasIO commented Oct 23, 2024

runtime errors 😬

will open a PR to rollback and work on fixing these.
The error you are running into is a type mismatch between manually constructed types and internal ffi types, glad we're running into these issues now instead of silently swallowing them. It basically means the change is working as intended, but I glanced over the runtime error possibility.

I'd prefer to merge #310 instead of the rollback, but the lack of exhaustive tests makes this slightly higher risk.

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