-
Notifications
You must be signed in to change notification settings - Fork 5
VIDCS-3721: improvements-documentation-video-api-example #163
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
base: develop
Are you sure you want to change the base?
VIDCS-3721: improvements-documentation-video-api-example #163
Conversation
VIDCS-3292: Push the new workflow and fixes to main
VIDCS-3367: Update main branch for VERA 1.0.1
VIDCS-3438: Update main branch for VERA 1.0.2
VIDCS-3583: Release Reference App 1.1.0
VIDCS-3711: Update main branch with VERA 1.1.1
|
||
Add your Vonage Video API credentials to the newly created .env file. |
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.
Add your Vonage Video API credentials to the newly created .env file.
It wasn't clear enough, -add your credentials- which credentials? from where and how can I get them?
Specificity on the steps make it more beginner friendly.
README.md
Outdated
<img src="./docs/assets/readme/1-dashboard-applications.png" alt="Applications dashboard" style="max-width: 100%; height: auto;" /> | ||
</details> | ||
|
||
If you don’t already have an application, create a new one: |
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.
steps how to create a new app if there is not already one. This info is present in some of the getting started documents but not in the example app
README.md
Outdated
|
||
Refer to the following image for visual guidance: | ||
|
||
<details open> |
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.
visual guidelines for the minimum configuration that the example app needs.
README.md
Outdated
In the root project directory, create the environment files by running: | ||
|
||
``` bash | ||
cp backend/.env.example backend/.env && cp frontend/.env.example frontend/.env |
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.
create both .env files in one command.
README.md
Outdated
|
||
#### Installing dependencies | ||
- **VONAGE_APP_ID** – This is the ID of your Vonage application. You can find it on the [Applications page](https://dashboard.vonage.com/applications). |
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.
specify which are the keys on the backend/.env that are required... for the basic configuration there is no need of modify the frontend/.env so better to don't mention it to avoid unnecessary complexity.
README.md
Outdated
```console | ||
#### Start in Development Mode | ||
|
||
``` bash | ||
yarn dev |
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 command remain the same, but the source was changed, please check the package.json
README.md
Outdated
|
||
#### Production mode | ||
### 5. Testing on Multiple Devices |
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.
adding extra step for testing with multiple devices on local network... a couple of simple extra steps allow the developer to test creating a conference with multiple devices, this is a more accurate real-word example more likely to catch the attention of whom is testing our API.
"start": "node --import tsx index.ts", | ||
"debug": "node --inspect --watch --import tsx index.ts", |
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.
command to debug the backend with chrome inspector
backend/server.ts
Outdated
@@ -27,10 +27,6 @@ app.use((_req, res, next) => { | |||
|
|||
app.use(express.static(path.join(dirName, './dist'))); | |||
|
|||
app.get('/*', (_req: Request, res: Response) => { |
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 can create confusion and is not API friendly, in an API if someone request a router that doesn't exists it should return not found... with this is returning basically and arbitrary html
@@ -7,7 +7,7 @@ | |||
"scripts": { | |||
"build": "vite build && yarn cp-build", | |||
"cp-build": "mkdir -p ../backend/dist && cp -r dist ../backend", | |||
"dev": "vite", | |||
"dev": "vite --host", |
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.
host is needed for the multiple devices example, and it doesn't affect the basic local example.
frontend/vite.config.ts
Outdated
return mergeConfig(vitestConfig, { | ||
server: { | ||
host: true, | ||
allowedHosts: ['*', env.VITE_NGROK_DOMAIN], |
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.
for local host "*" works, but for the ngrok domain we need to add it specifically otherwise vite will reject it... the steps for how and why to do this are on the new version of the readme
@@ -15,17 +15,19 @@ | |||
"scripts": { | |||
"build": "yarn workspace frontend build", | |||
"deploy-vcr": "vcr deploy", | |||
"dev": "yarn && yarn workspace frontend dev & yarn workspace backend dev", |
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 script was wrong, the intention was to run both project simultaneously but the terminal is not multiprocess by default.
we added concurrently as a dev dependency to achieve the desire result.
"docs": "yarn workspace frontend docs", | ||
"docs:watch": "yarn workspace frontend docs:watch", | ||
"lint": "ESLINT_USE_FLAT_CONFIG=false yarn eslint . --ext .ts,.tsx", | ||
"lint:filenames": "./scripts/lintFileNames.sh", | ||
"lint:fix": "yarn prettier --write . && yarn lint --fix", | ||
"postinstall": "husky", | ||
"run-server": "yarn workspace backend start", | ||
"start": "yarn workspace frontend build && yarn workspace backend start", |
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 backend run doesn't need to build the FE
"start:frontend": "yarn workspace frontend dev", | ||
"forward:frontend": "npx ngrok http 5173", |
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.
command for forward the port 5173 to be able to test the vite project as a https site.
@@ -68,6 +70,7 @@ | |||
"husky": "^9.0.11", | |||
"license-checker": "^25.0.1", | |||
"lint-staged": "^15.2.2", | |||
"concurrently": "^9.1.2", |
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.
allows to run multiple scripts in one terminal/command line
633ccdc
to
9d421cf
Compare
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.
great job so far! left a few comments. let me know what you think
README.md
Outdated
|
||
In project directory, create the environment variables for the project. | ||
To get started, make sure you have a Vonage account. You can create one at the [Vonage API Dashboard](https://dashboard.vonage.com/applications). The sign-up process is straightforward. |
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.
I think we can skip on mentioning the following: The sign-up process is straightforward.
README.md
Outdated
|
||
```console | ||
cp frontend/.env.example frontend/.env | ||
<details open> |
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.
I think this one and others should be closed
by default and not open
to be consistent and to make the page too crowded with the screenshots
README.md
Outdated
|
||
#### Dev mode | ||
This command installs all necessary dependencies. For details about the packages used, refer to [Dependencies](./docs/DEPENDENCIES.md). |
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 way it was worded before was fine - I don't think there is a need to change it.
README.md
Outdated
yarn start | ||
#### Steps: | ||
|
||
1. Create an account at [ngrok](https://dashboard.ngrok.com/signup) if you haven’t already (The free tier allows you to have one public tunnel). |
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.
I'd avoid mentioning what ngrok
allows on their free tier as that can change at any time. I'd also like to hear from other reviewers if this section is needed.
frontend/.env.example
Outdated
@@ -9,3 +9,7 @@ VITE_ENABLE_REPORT_ISSUE=false | |||
# This is meant for development only. | |||
# Note: this only works when opening a room url directly, not when navigating via the landing page. | |||
VITE_BYPASS_WAITING_ROOM=false | |||
|
|||
# For testing with multiple devices, you can use ngrok to expose your vite server to the internet. | |||
# your can see more information on how this works at https://ngrok.com/docs/ |
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.
I'd change this
your can see more information on how this works at https://ngrok.com/docs/
to something like this:
Please refer to https://ngrok.com/docs/ for more information.
I also noticed that integration tests are failing now. I wonder if it has to do with the changes that are done in the |
Removed the default * (catch-all) route for unfound API endpoints. This behavior was not API-friendly and caused confusion. Fixed project scripts: some were intended to run both the backend and frontend simultaneously, but Bash is not concurrent by default. We now use concurrently (via npm) to achieve the desired result. Added a script for debugging the backend project. Updated the README to make the tutorial more detailed and beginner-friendly. Readme now includes a clearer explanation of where and how to retrieve the necessary Vonage credentials. Added steps for testing the app with multiple devices on a local network.
9d421cf
to
f30300f
Compare
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.
Looks good so far! Just have a few comments/questions. Let me know what you think!
README.md
Outdated
@@ -114,49 +114,105 @@ The Vonage Video API Reference App for React is currently supported on the lates | |||
|
|||
## Running Locally | |||
|
|||
### Config | |||
### 1. Ensure You Have a Vonage Account |
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.
Could you re-generate the TOC? A few of the links changed in this PR are now broken. See the updated README here.
We've been using this extension.
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.
Hi @cpettet , thanks, is updated now.
README.md
Outdated
<img src="./docs/assets/readme/3-create-app-form.png" alt="Create app form" style="max-width: 100%; height: auto;" /> | ||
</details> | ||
|
||
### 3. Environment Variable |
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.
Nit
### 3. Environment Variable | |
### 3. Environment Variables |
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.
ready
README.md
Outdated
|
||
In project directory, create the environment variables for the project. | ||
To get started, make sure you have a Vonage account. You can create one at the [Vonage API Dashboard](https://dashboard.vonage.com/applications). |
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.
Nit, we're repeating the section heading. Maybe simplify to:
To get started, make sure you have a Vonage account. You can create one at the [Vonage API Dashboard](https://dashboard.vonage.com/applications). | |
You can create one at the [Vonage API Dashboard](https://dashboard.vonage.com/applications). |
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.
nice
frontend/.env.example
Outdated
|
||
# For testing with multiple devices, you can use ngrok to expose your vite server to the internet. | ||
# Please refer to https://ngrok.com/docs/ for more information. | ||
VITE_NGROK_DOMAIN=332a-45-137-35-107.ngrok-free.app |
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.
Could we remove this domain and use an empty string instead, similar to how we have the app ID?
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.
ready
package.json
Outdated
@@ -15,7 +15,7 @@ | |||
"scripts": { | |||
"build": "yarn workspace frontend build", | |||
"deploy-vcr": "vcr deploy", | |||
"dev": "yarn && yarn workspace frontend dev & yarn workspace backend dev", | |||
"dev": "concurrently 'yarn workspace frontend dev' 'yarn workspace backend dev'", |
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.
Could we add yarn
before this command so that all packages are installed? It's a little easier for us to just run yarn dev
.
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.
added
the integration test command needs to build and then start
b2fc2b5
to
cc0758c
Compare
attending feedback readme - fixing TOC - adding yarn to dev script, renaming VITE_NGROK_DOMAIN to VITE_TUNNEL_DOMAIN
|
What is this PR doing?
Removed the default * (catch-all) route for unfound API endpoints. This behavior was not API-friendly and caused confusion.
Fixed project scripts: some were intended to run both the backend and frontend simultaneously, but Bash is not concurrent by default. We now use concurrently (via npm) to achieve the desired result.
Added a script for debugging the backend project.
Updated the README to make the tutorial more detailed and beginner-friendly.
Readme now includes a clearer explanation of where and how to retrieve the necessary Vonage credentials.
Added steps for testing the app with multiple devices on a local network.
How should this be manually tested?
Follow the readme and the app should run
What are the relevant tickets?
Resolves VIDCS-3721
Checklist
[x] Branch is based on
develop
(notmain
).[ ] Resolves a
Known Issue
.[ ] If yes, did you remove the item from the
docs/KNOWN_ISSUES.md
?[ ] Resolves an item reported in
Issues
.If yes, which issue? Issue Number?