-
Notifications
You must be signed in to change notification settings - Fork 329
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
feat(templates): add Personal Voicemail template #107
base: master
Are you sure you want to change the base?
Conversation
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 in general :) I left a few comments. Thank you for the contribution!
personal-voicemail/README.md
Outdated
|
||
| Parameter | Description | Required | | ||
| :-------- | :---------- | :------- | | ||
| `phoneNumber` | The phone number to which you'd like to forward incoming calls, formatted in E164. | yes | |
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 it would be nice if we would support this via an environment variable as well. I think we use MY_PHONE_NUMBER
in a couple of the other templates.
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 have opinions about environment variables. ;)
When developing functions it is very easy for generic vars to clutter the environment, often to the point of clashing with one another. I feel that unless the function actually requires shared state or configuration that it should be kept local to the file. So unless a function requires multiple files to accomplish the task, or hooks in to a larger system's globals, then using environment variables is overkill, over complicating, and unnecessary.
Just in this repo, where all the functions are independent and not related to each other, most functions specify MY_PHONE_NUMBER
or TWILIO_PHONE_NUMBER
as the environment variable, thus forcing someone who wants to use function-templates with different phone numbers to structurally alter the code to make it function. To the less perceptive it may cause them to change the var and change the behavior of another function.
I'd rather see us provide details on how to update a function to use shared environment rather than make them the default.
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 definitely get your point. I think my only thought about environment variables vs query params is that they are more beginner friendly plus the serverless tooling is already letting people know that they should configure those variables.
Having environment variables with the same name can certainly be both a pro or a con. One of the things we considered when building the tooling was the ability to compose multiple templates together. For example you might start your project with twilio serverless:init example --template forward-call
which uses the MY_PHONE_NUMBER
. Later you want to be able to also forward your text messages. This would only require you to run inside the same project twilio serverless:new sms --forward-message
and after that you'd only have to configure the webhook URL without any other changes.
At the same time I can definitely also see the frustration of getting a clashing behavior here.
The other benefit of environment variables is that we could further improve the tooling to ask people if they intend to use the already configured env variable or not. That's a bit harder with query params.
The other thing with query params is that the event
can be both populated via query parameters and POST body and it can sometimes be confusing which one will be populated by Twilio and which one you have to set yourself.
I'm not too opinionated on either solution for your template. If you feel strongly about not offering configuration via env variables that's fine by me too. We just lack more documentation around query params than on environment variables atm.
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, too, I just noticed that you wrote
I think my only thought about environment variables vs query params is that they are more beginner friendly plus the serverless tooling is already letting people know that they should configure those variables.
This function uses hard coded vars in the function template and doesn't take any config options via get params. So there's nothing more needed than to update the function before it is deployed. I don't think I could advocate allowing for changing core configuration via GET params - sounds like a huge security issue. So I think this alleviates most of your concerns here around the discoverability of params... they're simply hard coded.
So maybe some clarification in this read me is in order.
Description
Provides a simple voice mailbox with SMS notifications and a block list. After callers leave a message you'll be sent a link to the recording via SMS.
An optional block list allows you to manually add phone numbers that you'd like to reject. Rejected callers are not allowed to leave messages.
Checklist
npm test
locally and it passed without errors.Related issues
N/A