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

Update gRPCweb example #33

Merged
merged 10 commits into from
Dec 7, 2023
Merged

Update gRPCweb example #33

merged 10 commits into from
Dec 7, 2023

Conversation

kupppo
Copy link
Contributor

@kupppo kupppo commented Dec 5, 2023

The examples/grpcweb folder currently only works with Windows. This PR updates a couple of things to address this:

  1. gen.sh has been changed to be executable. This was done with chmod +x ./gen.sh.
  2. gen.sh is now runs cross-platform. This previously failed due to the path of protoc-gen-ts having a hardcoded .cmd at the end which does not exist for Linux/Mac users.
  3. The generate script can now be called via npm. In keeping with npm conventions, the build script is now defined in the package.json file. Running npm run build will do the same thing as ./gen.sh.

I've also added a README.md to help get anyone looking to get started.

@kupppo kupppo marked this pull request as ready for review December 5, 2023 16:34
@JamesDunne
Copy link
Contributor

lgtm! I'll run it locally first and merge if it looks good. thank you.

@kupppo
Copy link
Contributor Author

kupppo commented Dec 6, 2023

This has been tidied up a bit and removed old dependencies. I've confirmed it runs on Windows, M1 Mac and Intel Mac.

@JamesDunne
Copy link
Contributor

I like the new cleanups but I think you went one step too far with removing src/index.ts.

The original intention of this grpcweb example was to be a minimal working example of how to use grpc-web to talk to SNI directly from a web browser, which isn't normally possible with vanilla grpc.

@kupppo
Copy link
Contributor Author

kupppo commented Dec 6, 2023

No worries. I got caught up in cleaning up that I forgot that this is an example and not a package. I'll get a demo with this working on a basic web site and re-request review.

@kupppo
Copy link
Contributor Author

kupppo commented Dec 7, 2023

@JamesDunne This is ready for review again. I've used Next.js to make an example page that uses DeviceList. There are instructions in the README for how to get the page to run. Here are some screenshots of the example page running.

Screenshot 2023-12-07 at 12 41 07 PM Screenshot 2023-12-07 at 12 41 46 PM Screenshot 2023-12-07 at 12 42 09 PM

@JamesDunne JamesDunne merged commit 246e6aa into alttpo:main Dec 7, 2023
@kupppo kupppo deleted the grpcweb-update branch December 7, 2023 20:52
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