Skip to content

Conversation

@nbsp
Copy link

@nbsp nbsp commented Nov 13, 2024

this PR changes the CI to run two protobuf generators, one for each, plus another for dts. currently untested.

aoife cassidy added 2 commits November 13, 2024 23:47
this PR changes the CI to run two protobuf generators, one for each,
plus another for dts. currently untested.
@nbsp nbsp requested a review from lukasIO November 13, 2024 21:53
@changeset-bot
Copy link

changeset-bot bot commented Nov 13, 2024

⚠️ No Changeset found

Latest commit: c89b867

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

💥 An error occurred when fetching the changed packages and changesets in this PR
Some errors occurred when validating the changesets config:
The package or glob expression "github.com/livekit/protocol" specified in the `fixed` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

export * from "./gen/livekit_sip_pb.js";
export * from "./gen/livekit_webhook_pb.js";
export * from "./gen/version.js";
export type * from "./gen/dts/livekit_agent_dispatch_pb.d.ts";
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, don't remember ever seeing this import pattern 🤔

Copy link
Author

Choose a reason for hiding this comment

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

the LSP didn't complain, but i'll double check to make sure this works

Copy link
Author

Choose a reason for hiding this comment

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

works great, except the files under cjs need to be renamed to .cjs in order to be required as CommonJS. i don't know if GH Actions comes built in with any rename utility

Copy link
Author

Choose a reason for hiding this comment

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

ok so i did a bulkrename but now i need to sed through all the files and replace the names as well

Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice if we could configure that as part of the protocol es plugin, I think file extension is an option there?

Copy link
Author

Choose a reason for hiding this comment

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

only able to set js/ts/d.ts on there unfortunately

@nbsp nbsp requested a review from lukasIO November 13, 2024 23:24
aoife cassidy added 2 commits November 14, 2024 01:26
@nbsp
Copy link
Author

nbsp commented Nov 13, 2024

tested working on livekit-server-sdk (which breaks for a different reason, will be handled there, this part is fine)

export * from "./gen/livekit_sip_pb.js";
export * from "./gen/livekit_webhook_pb.js";
export * from "./gen/version.js";
export type * from "./gen/dts/livekit_agent_dispatch_pb.d.ts";
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice if we could configure that as part of the protocol es plugin, I think file extension is an option there?

@lukasIO
Copy link
Contributor

lukasIO commented Nov 14, 2024

before merging this, it would be great if we could test and verify the ESM and CJS behaviour with the most common bundlers/frameworks

  • NestJS
  • NextJS
  • webpack
  • rollup
  • esbuild

@nbsp
Copy link
Author

nbsp commented Nov 26, 2024

tested building downstream with esbuild (agents + tsup)

@nbsp nbsp requested a review from lukasIO November 26, 2024 16:07
@nbsp nbsp requested a review from lukasIO November 27, 2024 08:32
Copy link
Contributor

@lukasIO lukasIO 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 so much nicer, great work

@nbsp nbsp merged commit 00cc444 into main Nov 27, 2024
3 checks passed
@nbsp nbsp deleted the nbsp/chore/cjs branch November 27, 2024 10:45
@github-actions github-actions bot mentioned this pull request Nov 27, 2024
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.

3 participants