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

Avoid typescript warning when no roles key is supplied #6060

Merged
merged 1 commit into from
Jul 28, 2022

Conversation

Philzen
Copy link
Contributor

@Philzen Philzen commented Jul 27, 2022

Line 108 indicates that this method can be used without supplying roles at all:

However, this would trigger a typescript warning:

Argument of type '{}' is not assignable to parameter of type '{ roles: AllowedRoles; }'.
  Property 'roles' is missing in type '{}' but required in type '{ roles: AllowedRoles; }'.ts(2345)
auth.ts(101, 42): 'roles' is declared here.

This fixes it.

@nx-cloud
Copy link

nx-cloud bot commented Jul 27, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 4707b98. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 14 targets

Sent with 💌 from NxCloud.

@netlify
Copy link

netlify bot commented Jul 27, 2022

Deploy Preview for redwoodjs-docs canceled.

Name Link
🔨 Latest commit 4707b98
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/62e19cad00746100075b7076

@Philzen
Copy link
Contributor Author

Philzen commented Jul 27, 2022

BTW – Currently, the method throws when called without any argument, so one always has to supply {} and cannot pass the method around without this as a pre-bound parameter.

Hence, my suggestion / wish for v3.0 would be to change this method from destructuring to simply use a discrete argument.

The hasRole-method signature was actually changed exactly that way in 84460eb, so it would only make sense to harmonize these APIs.

Would love to hear the core-team's thoughts on this.

@simoncrypta simoncrypta added the release:fix This PR is a fix label Jul 28, 2022
Copy link
Collaborator

@simoncrypta simoncrypta left a comment

Choose a reason for hiding this comment

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

Thanks for this fix, that make sense!

@simoncrypta
Copy link
Collaborator

@Philzen For your suggestion, maybe @cannikin had some thought on it ?

@simoncrypta simoncrypta merged commit ac81566 into redwoodjs:main Jul 28, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jul 28, 2022
@Philzen Philzen deleted the fix-optional-roles-type branch July 28, 2022 03:07
@jtoar jtoar modified the milestones: next-release, next-release-patch Aug 1, 2022
@Philzen
Copy link
Contributor Author

Philzen commented Aug 7, 2022

@Philzen For your suggestion, maybe @cannikin had some thought on it ?

@simoncrypta Actually #6110 fixed the limitation i described, which is awesome, now we can simply pass requireAuth to validateWith in our service methods!

Now there's really no big gain in changing the method signature of requireAuth, other than having harmonized method signatures of course. Though it is still interesting to learn why hasRoles signature was changed to the favorable, simpler version in 84460eb, but requireAuth was not.

@jtoar jtoar modified the milestones: next-release, v3.0.0 Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants