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

update Joi namespace and move to latest version #235

Merged
merged 1 commit into from Aug 30, 2019
Merged

update Joi namespace and move to latest version #235

merged 1 commit into from Aug 30, 2019

Conversation

danieladams456
Copy link
Contributor

@danieladams456 danieladams456 commented Jun 28, 2019

All tests still pass, closes #234

Edit: After additional tests in #240, the wreck upgrade now breaks the build. Switching the PR to just do joi for now, and then the wreck issue can be solved on another.

@gal1419
Copy link

gal1419 commented Jul 2, 2019

any plans to merge this PR anytime soon?

@mitramejia
Copy link

this is needed to work with latests node versions since old wreck packages uses the old node < v8 Url which has been removed by a newer node version

@jonathansamines
Copy link
Collaborator

Hi @danieladams456 I have recently reviewed this pull request. Unfortunately seems like Wreck 15 introduced some breaking changes that may impact how this library resolve urls. I have added some tests at #240 to make sure the existing behavior is not broken by this upgrade. Can you try syncing master (or that branch) with your tests and verify that everything is still working?

@jonathansamines
Copy link
Collaborator

@mitramejia I am not aware of any module being removed from Node.js. url has been deprecated in newer versions but that is different from removing an API. Which version of Node.js are you using?

@danieladams456
Copy link
Contributor Author

@jonathansamines good catch. It seems like the build is failing now. I was just doing a POC when I ran across this project and submitted the PR. Is there some way to transfer it to someone else? Otherwise the diff is really simple so could be closed and re-created. Thank you!

@DaKaZ
Copy link

DaKaZ commented Aug 29, 2019

@danieladams456 do you just want to see the PR through - I don't think you need to do anything beyond resolving the merge conflicts and making sure the tests pass.

@danieladams456 danieladams456 changed the title update hapi modules namespace and move to latest version update Joi namespace and move to latest version Aug 29, 2019
@danieladams456
Copy link
Contributor Author

@DaKaZ I gave it another shot this evening hoping my rebase against package-lock.json was the issue, but using npm to install wreck still breaks the tests. I switched this PR just to upgrade joi, and then the wreck issues can be solved on another PR. @jonathansamines should be ready to merge now, sorry I couldn't be of more help.

All three grant types have this same issue:

  1) authorization code grant type
       when requesting an access token
         with custom token host and path
           "before all" hook:
     Error: Client request error: Nock: No match for request {
  "method": "POST",
  "url": "https://authorization-server.org/oauth/token",
  "headers": {
    "accept": "application/json",
    "authorization": "Basic dGhlK2NsaWVudCtpZDp0aGUrY2xpZW50K3NlY3JldA==",
    "content-type": "application/x-www-form-urlencoded",
    "host": "authorization-server.org",
    "content-length": 78
  },
  "body": "code=code&redirect_uri=http%3A%2F%2Fcallback.com&grant_type=authorization_code"
}

@jonathansamines jonathansamines merged commit a90f9e6 into lelylan:master Aug 30, 2019
@jonathansamines
Copy link
Collaborator

@danieladams456 Thanks for putting the effort into making this update. Will handle the Wreck update in a separate issue.

Thanks!

@danieladams456
Copy link
Contributor Author

@jonathansamines BTW thought you did a great job on TDD with those tests you put in to catch anticipated breaking changes. 😃

@DaKaZ
Copy link

DaKaZ commented Sep 13, 2019

@danieladams456 I just wanted to say thanks! Also thanks @jonathansamines for getting this merged and a new npm published. Solved my issues :)

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.

Moved modules
5 participants