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

chore(deps): Upgrade hapi/joi dependency #12885

Merged
merged 1 commit into from
May 25, 2022
Merged

chore(deps): Upgrade hapi/joi dependency #12885

merged 1 commit into from
May 25, 2022

Conversation

xlisachan
Copy link
Contributor

@xlisachan xlisachan commented May 15, 2022

Initial review - #12578

Because

  • the hapi/joi dependency is deprecated and several versions of it was used throughout the codebase

This pull request

  • upgrades hapi/joi dependency to joi v17.4.0 (note: while joi v17.6.0 is the latest version, upgrading to joi 17.6.0 causes a "Cannot mix different versions of joi schemas" since celebrate uses joi 17.4.0.)
  • updates outdated joi usage - confirmed with Reino that there is no need to revise where error !== null as "when fxa-payments-server gets an error from the backend, it throws an APIError"
  • updates hapi-swagger and its plugins

Issue that this pull request solves

Closes: #12178
Closes: #12477
Closes: #12528

Put an x in the boxes that apply

  • My commit is GPG signed.

Follow-up tickets

- Investigate typesafe-joi dependency and better solution as it is a fork of hapi/joi v15.x

@xlisachan xlisachan force-pushed the FXA-4724_joi branch 2 times, most recently from 5a02709 to 3b15b34 Compare May 17, 2022 13:56
@xlisachan xlisachan force-pushed the FXA-4724_joi branch 4 times, most recently from 0d0706b to ed68aee Compare May 23, 2022 20:21
Copy link
Contributor

@dschom dschom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just remove that one unneeded async statement and it is good to go.

packages/fxa-auth-server/test/oauth/routes/token.js Outdated Show resolved Hide resolved
@xlisachan xlisachan force-pushed the FXA-4724_joi branch 6 times, most recently from 0bf2a92 to a7a73d9 Compare May 24, 2022 21:55
@xlisachan xlisachan marked this pull request as ready for review May 24, 2022 22:10
@xlisachan xlisachan requested a review from a team as a code owner May 24, 2022 22:11
@xlisachan xlisachan changed the title chore(deps): Upgrade hapi/joi dependency - WIP chore(deps): Upgrade hapi/joi dependency May 24, 2022
@xlisachan
Copy link
Contributor Author

xlisachan commented May 24, 2022

Please note, the first commit (Upgrades joi, hapi-swagger and its plugins) has been approved by Dan.

The second commit (Rewrite typesafe-joi and remove it from package.jsons; still exists in fxa-event-broker) needs to be reviewed. This commit needed to fix a Cannot mix different versions of joi schemas error because we were mixing validation from joi and validation using typesafe-joi which is a fork of @types/hapi__joi and matches the API of hapi/joi v15.x in fxa-shared. Therefore, the following changes have been made:

  • Rewrote exported types that were set to typesafe-joi's joi.Literal<>
  • Removed typesafe-joi from package.json files where it was no longer used. Note it is still being used in fxa-event-broker

Also, I tried upgrading to joi v17.6.0 (the latest version) again, but celebrate and @hapi/catbox-redis use joi v 17.4.0, which causes a Cannot mix different versions of joi schemas: 17.6.0 and 17.4.0 error (e.g., in server/lib/server.test.js).

Thanks! :)

@dschom dschom self-requested a review May 24, 2022 23:36
@xlisachan
Copy link
Contributor Author

@dschom, please let me know if you have any additional feedback or whether it's good to merge as I don't believe you can mark this PR as approved again after your initial review. Thanks! :)

@xlisachan
Copy link
Contributor Author

(Dan said it's gtg, rebasing and merging! \o/)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants