@travi Thanks for the change and sorry for being so unresponsive.

Can you change this so there is a flag in the config which turns this feature on?

I think that makes sense, but I have a couple small questions before I dive back in:

• Do you have a preference about what the config flag should be? appendRaw seems like it would align fairly well
• What should the default value be for the flag?
  • appendRaw: true is the behavior that I personally would have expected to be the natural default. while there may not be a lot of people out there using setUrl(), which should be the only time the value of this flag would have an impact, i would have expected to be redirected back to the original request rather than the re-written request. with this default, we would basically be saying that the previous behavior was a bug, rather than expected behavior. if this is the default, how realistic is it that someone would need to change that behavior? it seems unlikely enough that I do wonder if the flag is actually needed.
  • appendRaw: false would maintain the previous behavior, so it would not break anyone that uses setUrl() and was expecting the previous behavior. However, it seems like this would trip up anyone in the future because they would need to flip an additional flag to get what I assume would be their expected behavior.

@mrlannigan what would it take to move this forward? I'm still leveraging my own fork in order to work around this limitation for my own projects, but would love for there to be a way to use this from this project directly.

do you support the solution in its current form, or do think there are changes necessary?

Hello @travi,

Thank you first off for your contribution!

I would like to keep this functionality non-breaking, if possible. This is mainly because I feel like the difference between it being raw or not, could end up being a subtle one. If you think I'm crazy tell me. 😉

As far as the config, how would you feel about possibly extending appendNext to be an object as well? For example:

{
  appendNext: {
    raw: true,
    name: 'next'
  }
}

(I added name just to demonstrate a way to set the uri parameter name with an object)

This would mean that appendNext could be a Boolean, String, or an Object.

i think the object approach makes sense. i do think the raw approach is actually what is truly needed in most cases, but i'm not deep enough into other projects to say that with enough confidence to try to sway you (especially since that would end up being a breaking change).

i looked at rebasing these changes against master, but realized that things have changed a fair bit with updating to be hapi v17 compatible. since we're talking about keeping this change backward-compatible anyway, would you consider integrating the change at a point in the history that was before the hapi v17 changes? that would make getting it rebased simpler, but would also enable this functionality for apps that arent yet upgraded (mine included). if we went that route, i'd be happy to follow up to get a PR together to get it from there into master.

let me know if a new branch that is pre-v17 or master is where you'd want to start and i can work toward getting this updated and incorporate the changes you mentioned above.

@mrlannigan i'm hoping to get a chance to update this PR within the week, incorporating your suggestions.

are you open to the idea of integrating to a pre-hapiv17 version first and then migrating to master? if so, would you mind creating a branch that i could target this PR at and rebase my changes with?

Sounds reasonable as long as we are able to get this change into current master as well.

Branch created, v7-pre17.

great! i'll work on getting things working and updated to target that branch to start with.

i'm on board for making sure we follow through to get this into master as well. i'll still need it once we do get our projects updated to hapi v17.

first question comes immediately after rebasing my branch with the branch you created and installing the proper dependencies. when i run npm test, i see the following error, which causes an exit code of 1: The following leaks were detected:WebAssembly

this is on node v8.9.4 and npm v5.6.0. anything obvious to you why this would be failing? this PR obviously has nothing to do with web assembly.

i think this should be good to go, behavior wise. i added details to the readme about the change as well.

let me know if there is anything about the contribution that you don't feel aligns to the existing style of the codebase. i'm happy to adjust.

@mrlannigan any adjustments you'd like to see here?

@mrlannigan i'd love to see this get merged so i can start on that follow up for hapi v17. Anything preventing this from moving forward?

Version 7.1.0 released to npm

thank you for getting this merged and published.

i can try to get a PR for this behavior against hapi v17 soon. anything in particular that i need to pay specific attention to outside of the hapi api changes?

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.