Add new server

[timmydoza]

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

• I acknowledge that all my contributions will be made under the project's license.

Pull Request Details

JIRA link(s):

• AHOYAPPS-3891

Description

This PR adds a new token server to the React app, which can be used for local development and to drive the Cypress E2E tests. This server imports the token function from the RTC Twilio CLI Plugin. This means that this server has the same API and functionality as the Serverless Functions that are deployed by the CLI plugin.

With this change, this server now has support for Twilio Conversations, which means that it will now be possible to build the Conversations UI and test it in Cypress.

Burndown

Before review

• Updated CHANGELOG.md if necessary
• Added unit tests if necessary
• Updated affected documentation
• Verified locally with npm test
• Manually sanity tested running locally
• Included screenshot as PR comment (if needed)
• Ready for review

Before merge

• Got one or more +1s
• Re-tested if necessary

[charliesantos]

Left a couple of comments. Also, I'm on the fence on this approach where we are mocking some globals so that the serverless function works in an express environment. If we continue to follow this, it means future development mindset would be:

• Write serverless function and make sure it works in the serverless environment.
• Update express app and mock any globals so that the new serverless function works ok.
• Any new updates on the function that might break the express app (e.g. adding new globals) requires updating the express app.

I think the right approach would be to write functions/endpoints that works on both platforms (serverless and express) that way we don't have to mock globals. If you look at other cross platform code, this is usually how they handle this. Thoughts?

[charliesantos]

Why do we need to compile the ts files if we are already using ts-node?

[timmydoza]

I added this script so that we could check for any errors during our Circle CI builds. I suppose we only need to perform type checking, not compilation. I'll change the script name to better describe this and also add --noEmit.

[charliesantos]

Please update .env.example

[charliesantos]

Can we also add CORS headers?

app.use((req, res, next) => {
  res.header('Access-Control-Allow-Origin', '*');
  res.header('Access-Control-Allow-Headers', '*');
  next();
});

[timmydoza]

Sure - just curious though, why would we want to add them? I'm planning on hosting the front-end assets from the same origin as this server, so I don't think we would need them.

[charliesantos]

I was thinking for dev purposes only. I usually have different frontend accessing different servers locally. This is one of them :)
But it's ok not to add them if we're going to deploy this.

[timmydoza]

I see. When the app is run in dev mode with npm start, the api requests made to /token will be proxied to localhost:8081/token. So from that app's point of view, everything is on the same origin in dev mode. So I don't think we'll need them for that. But if we ever need them in the future I'm happy to add them in.

[timmydoza]

Left a couple of comments. Also, I'm on the fence on this approach where we are mocking some globals so that the serverless function works in an express environment. If we continue to follow this, it means future development mindset would be:

• Write serverless function and make sure it works in the serverless environment.
• Update express app and mock any globals so that the new serverless function works ok.
• Any new updates on the function that might break the express app (e.g. adding new globals) requires updating the express app.

I think the right approach would be to write functions/endpoints that works on both platforms (serverless and express) that way we don't have to mock globals. If you look at other cross platform code, this is usually how they handle this. Thoughts?

I definitely understand the concern here, but I think that it will be somewhat rare that we will run into steps 2 and 3 from the list above. When we update our Serverless functions in the future (or add new ones), I think it is unlikely that we will need to update the global variables. We might need to add new assets here, but other than that I can't think of any globals that we would need to add or change in the future. It's possible that I missed one or two, but I don't think that adding them in later would be a high maintenance cost.

I think the right approach would be to write functions/endpoints that works on both platforms

I agree that this would be ideal, but unfortunately, I don't see how it's possible. The only way to change the status code and headers of a response from a Twilio Serverless function is to use the global Twilio.Response. Because Twilio Functions have a hard dependency on the Twilio global, I don't see how we can get around mocking it if we want these functions to run in Express too.

In my mind, the main thing that we want to avoid in building a new server is duplicating business logic (and tests for the same) across multiple repos, which is what we are currently have with video-app-service and plugin-rtc. With the approach in this PR, all of the business logic (and tests) live in plugin-rtc, and this server has code to run them as part of an Express server. So even though we may need to update these globals in the future, I think that we are still saving quite a lot in terms of long-term maintenance costs.

[charliesantos]

Thanks for addressing my concerns! let me know once this is ready for another look.