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

add query to twimlFor, backgroundTrigger, and transitionOut #50

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

data-doge
Copy link
Collaborator

eyyy if this is something you'd like in twilio-ivr, lmk, and i can update your docs.

otherwise, feel free to close

@ethanresnick
Copy link
Owner

I'm gonna leave this open because I'm not totally opposed to it, at least as a stopgap, but I think you might run into some issues with this approach. I'd say try it out on the hotline repo and update the PR as needed; once you get the hotline migrated to this successfully and work out any kinks, then we can talk about merging it.

Among the current issues I see:

  1. How can transitionOut (and backgroundTrigger) add to the query params/session? Does it (they) need to?
  2. This structure seems likely to lead to bugs: if any normal state's twimlFor forgets to add the query params to the processTransitionUrl, the chain will break. Maybe at least a helper can be written to aid with that?
  3. The query params need to be passed along here too.

@ethanresnick
Copy link
Owner

@data-doge fyi, i moved some files around today—looking at the code through your eyes made me realize how badly defined the boundaries were between various subcomponents—so this pr will need some updating. almost none of the logic has changed, though, just which files are where has changed, so the updates should be very easy/mechanical.

@ethanresnick
Copy link
Owner

@data-doge just saw that the latest tweaks here add a trustProxy config setting to twilio-ivr. For the sake of merging this PR eventually, I thought I'd say now that I'm not sure that's the best approach.

Part of me is hesistant to add express-configuration logic to twilio-ivr at all; it seems out of scope. I think maybe a preferred solution would be to configure an external app, and then have that app use twilio-ivr? I.e.:

// in user-land code, configure app
const app = express();
app.set('trust-proxy', true);

// add twilio-ivr
app.use(twilioIvr(states, config));

Does that feel too clunky to you? If so, I could also maybe see accepting an serverConfig setting in twilio-ivr that gets passed along to express, like we used to do here. Adding one-off setting flags doesn't seem scalable though, if we have to add more and more over time or if we want to generalize twilio-ivr to work with different server frameworks (like hapi).

@data-doge
Copy link
Collaborator Author

@ethanresnick

yeah, external app sounds like a great idea. feels less prescriptive.

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