twilio-ivr icon indicating copy to clipboard operation
twilio-ivr copied to clipboard

add query to twimlFor, backgroundTrigger, and transitionOut

Open data-doge opened this issue 7 years ago • 4 comments

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

otherwise, feel free to close

data-doge avatar Apr 04 '17 02:04 data-doge

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 avatar Apr 04 '17 03:04 ethanresnick

@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 avatar Apr 05 '17 01:04 ethanresnick

@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).

ethanresnick avatar Apr 19 '17 19:04 ethanresnick

@ethanresnick

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

data-doge avatar Apr 19 '17 20:04 data-doge