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

referenced the raw request when setting next #151

Merged
merged 1 commit into from May 27, 2018
Merged

Conversation

@travi
Copy link
Contributor

travi commented Dec 6, 2016

this would be a breaking change for anyone that uses setUrl(), but does not want the original request to be where next is directed. i struggle to think of such a case, but you likely have better insight.

resolves #150

@travi travi changed the title referrenced the raw request when setting next referenced the raw request when setting next Dec 6, 2016
@travi travi force-pushed the travi:raw branch to 5fcdd9c Dec 7, 2016
@jaw187

This comment has been minimized.

Copy link
Contributor

jaw187 commented Feb 17, 2017

@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?

@jaw187 jaw187 self-assigned this Feb 17, 2017
@jaw187 jaw187 added the feature label Feb 17, 2017
@travi

This comment has been minimized.

Copy link
Contributor Author

travi commented Feb 17, 2017

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

This comment has been minimized.

Copy link
Contributor Author

travi commented Jan 18, 2018

@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?

@mrlannigan mrlannigan added this to the 8.1.0 milestone Jan 21, 2018
@mrlannigan

This comment has been minimized.

Copy link
Contributor

mrlannigan commented Jan 21, 2018

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.

@travi

This comment has been minimized.

Copy link
Contributor Author

travi commented Jan 24, 2018

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.

@travi

This comment has been minimized.

Copy link
Contributor Author

travi commented Feb 2, 2018

@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?

@mrlannigan

This comment has been minimized.

Copy link
Contributor

mrlannigan commented Feb 8, 2018

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

Branch created, v7-pre17.

@mrlannigan mrlannigan modified the milestones: 8.1.0, 8.2.0 Feb 8, 2018
@travi travi changed the base branch from master to v7-pre17 Feb 9, 2018
@travi travi force-pushed the travi:raw branch from 5fcdd9c to 006f935 Feb 9, 2018
@travi

This comment has been minimized.

Copy link
Contributor Author

travi commented Feb 9, 2018

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.

@travi

This comment has been minimized.

Copy link
Contributor Author

travi commented Feb 9, 2018

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.

resolves #150
@travi travi force-pushed the travi:raw branch from 006f935 to 5b1f669 Feb 10, 2018
@travi

This comment has been minimized.

Copy link
Contributor Author

travi commented Feb 10, 2018

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.

@travi

This comment has been minimized.

Copy link
Contributor Author

travi commented Feb 25, 2018

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

@travi

This comment has been minimized.

Copy link
Contributor Author

travi commented Mar 24, 2018

@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?

@mrlannigan mrlannigan merged commit a4f07b6 into hapijs:v7-pre17 May 27, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mrlannigan

This comment has been minimized.

Copy link
Contributor

mrlannigan commented May 27, 2018

Version 7.1.0 released to npm

@mrlannigan mrlannigan mentioned this pull request May 27, 2018
@travi

This comment has been minimized.

Copy link
Contributor Author

travi commented May 29, 2018

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?

travi added a commit to travi/hapi-auth-cookie that referenced this pull request Jun 5, 2018
fixes hapijs#150 by applying hapijs#151 to hapi v17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.