-
Notifications
You must be signed in to change notification settings - Fork 216
Create React Version of the js/Ice/Greeter Demo #514
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
Conversation
Co-authored-by: bernardnormier <[email protected]>
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review and fix @pepone comments.
…ator management Co-authored-by: bernardnormier <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
Make sure every file added by this PR ends with a newline character. |
Co-authored-by: bernardnormier <[email protected]>
Added missing newlines to all files in the greeter-react demo in commit 5d79ec3. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- state message boxes have too much bottom padding.
- entering no name after getting a successful response doesn't dismiss the previous server response
js/Ice/greeter-react/README.md
Outdated
| - Navigate to the demo directory: | ||
|
|
||
| ```shell | ||
| cd js/Ice/greeter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dir is incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, renamed to react-greeter for consistency with android-greeter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The directory path js/Ice/react-greeter is correct - the demo was renamed from greeter-react to react-greeter in recent commits.
Cleanup the css
Updated the demo to use a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request implements a new React-based version of the Ice Greeter demo, integrating modern frameworks and tooling (Vite, TypeScript, React 18) with Ice for communication.
- Introduces a new directory for the React demo with full integration of Ice including proper error handling and loading states.
- Adds updated documentation and configuration files to support a modern build system and browser environment.
- Updates the main README to include the new React demo alongside existing demos.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| js/README.md | Updated demos table to include the new Ice React Greeter demo. |
| js/Ice/react-greeter/vite.config.ts | New Vite configuration for the React application. |
| js/Ice/react-greeter/tsconfig.json | New TypeScript configuration tailored for a Vite and React environment. |
| js/Ice/react-greeter/src/* | Entire React application including main entry points, styling, and status messaging. |
| js/Ice/react-greeter/Greeter.ice | Greeter Slice definition for the React demo. |
| js/Ice/react-greeter/package.json | Dependency and script definitions for the React demo. |
| js/Ice/react-greeter/index.html | HTML entry point for the React application. |
| js/Ice/react-greeter/README.md | Documentation updated to guide development and usage of the React demo. |
| js/Ice/react-greeter/.npmrc | NPM registry configuration for ZeroC packages. |
| js/Ice/react-greeter/.gitignore | Git ignore configuration for generated and build files. |
| js/Ice/greeter/README.md | Minor documentation updates in the existing Greeter demo. |
Comments suppressed due to low confidence (1)
js/Ice/react-greeter/src/App.tsx:35
- Double-check that instantiating the Greeter proxy using 'new' is consistent with the expected API for the @zeroc/ice package, as factory methods are sometimes preferred.
const greeter = new VisitorCenter.GreeterPrx(communicator, "greeter:ws -h localhost -p 4061");
js/Ice/react-greeter/src/App.tsx
Outdated
| })); | ||
| } | ||
|
|
||
| return () => { |
Copilot
AI
Jun 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider explicitly handling the promise returned by the asynchronous cleanup function within the useEffect cleanup to ensure that any potential errors during communicator destruction are properly managed.
… name Co-authored-by: pepone <[email protected]>
Fixed both issues in commit 55a6a54:
|
js/Ice/react-greeter/src/App.tsx
Outdated
| const cleanup = async () => { | ||
| try { | ||
| if (communicatorRef.current) { | ||
| await communicatorRef.current.destroy(); | ||
| } | ||
| } catch (err) { | ||
| console.warn("Failed to destroy communicator:", err); | ||
| } | ||
| }; | ||
| // Kick off the async cleanup, but don't return the promise | ||
| cleanup(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this extra step necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check, it is definitely ugly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The callback returned by useEffect must be not async
src/App.tsx:27:15 - error TS2345: Argument of type '() => () => Promise<void>' is not assignable to parameter of type 'EffectCallback'.
Type '() => Promise<void>' is not assignable to type 'void | Destructor'.
Type '() => Promise<void>' is not assignable to type 'Destructor'.
Type 'Promise<void>' is not assignable to type 'void | { [UNDEFINED_VOID_ONLY]: never; }'.
27 useEffect(() => { ~~~~~~~
If we call destroy directly and not await it we can end up with an unhalted promise rejection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another alternative is:
return () => {
const communicator = communicatorRef.current;
communicatorRef.current = null;
// Clean up the communicator when the component unmounts
if (communicator) {
communicator.destroy().catch((err) => {
console.error("Error destroying Ice communicator:", err);
});
}
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either one is fine with me.
js/Ice/react-greeter/src/App.tsx
Outdated
| @@ -0,0 +1,124 @@ | |||
| import React, { useState, useCallback, useEffect, useRef } from "react"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing copyright
js/Ice/react-greeter/src/App.tsx
Outdated
|
|
||
| import StatusMessage from "./StatusMessage"; | ||
|
|
||
| interface AppState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too few comments
| @@ -0,0 +1,29 @@ | |||
| import React from "react"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs copyright and comments
This PR implements a React-based version of the existing
js/Ice/greeterdemo as requested in the issue.What's Added
js/Ice/greeter-react/directory with a complete React application@zeroc/icepackage from the ZeroC nightly NPM feedKey Features
The React application provides:
Technical Implementation
Greeter.iceSlice definition as the original demoUsage
npm installnpm run buildnpm run devThe application expects a Greeter server to be running on
localhost:4061(same as the original demo).Documentation Updates
Updated
js/README.mdto include the new React demo in the demos table.Fixes #513.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
http://168.63.129.16:80/machine//usr/bin/python3 -u bin/WALinuxAgent-2.13.1.1-py3.9.egg -collect-logs(http block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.