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

Register Express as a peer dependency #63

Merged
merged 2 commits into from
Feb 7, 2020
Merged

Conversation

stevehobbsdev
Copy link

@stevehobbsdev stevehobbsdev commented Feb 7, 2020

Description

This PR:

  • Adds Express as a peer dependency ">= 4.17.0"
  • Corrects the minimum Express version in the readme
  • Corrects the sample in the readme, where appSessionKey should be appSessionSecret

Rationale

The peer dependency of Express is useful because:

  • The minimum required version of Express is 4.17, which brings in cookie@0.4.0, which is the minimum version of cookie that supports the SameSite attribute
  • An app scaffolded with express-generator still installs express@4.16 by default which brings in a lower version of cookie that does not support the SameSite attribute
  • Including express@>= 4.17.0 as a peer dependency means that at least the user will get a warning when they try to install it
  • As this library is primarily used in an Express application, we can leave the local dependency on express in devDependencies

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

@stevehobbsdev stevehobbsdev added the review:tiny Tiny review label Feb 7, 2020
@stevehobbsdev stevehobbsdev changed the title Fix/peer dependency Register Express as a peer dependency Feb 7, 2020
@stevehobbsdev stevehobbsdev requested a review from a team February 7, 2020 11:07
@lbalmaceda
Copy link
Contributor

@stevehobbsdev, please rebase

@joshcanhelp
Copy link
Contributor

@stevehobbsdev - Thanks for this! The Quickstart and sample need to be updated as well

@stevehobbsdev
Copy link
Author

@joshcanhelp What needs to happen on the sample? I can't see where these changes would affect anything there. Will submit a change to the quickstart now.

@lbalmaceda I've rebased, looks like CI is fixed 👍

@stevehobbsdev
Copy link
Author

@joshcanhelp Actually it looks like the quickstart is correct.

@lbalmaceda
Copy link
Contributor

@stevehobbsdev
Copy link
Author

Done, PRs attached.

@lbalmaceda
Copy link
Contributor

Both merged now. I'll merge this one as well 👍

@lbalmaceda lbalmaceda merged commit c918849 into master Feb 7, 2020
@lbalmaceda lbalmaceda deleted the fix/peer-dependency branch February 7, 2020 19:46
@joshcanhelp joshcanhelp added this to the v0.7.0 milestone Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants