-
Notifications
You must be signed in to change notification settings - Fork 0
Chore/clientside sessions #472
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
Conversation
…expose server side constructors to client
theodorklauritzen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good after the issue discussed are fixed
…d on the nonchanging methods in editModeCtx
|
This I now fixed @theodorklauritzen |
|
Looks really good. Just one small thing. I never really liked the word "auther". I always read "author", its non standard from what i can see, and it only saves four letters compared to the much clearer "authorizer". I feel like for new people looking over the code, seeing something like this const canEdit = useEditMode({ auther })will look like a spelling mistake. May i suggest switching to "authorizer" now that we have the chance? I have done this in the branch |
|
I agree. We can change it. Keeping names short has no purpose in my opinion. Just one question: the property authorizer often refers to some different things now: are the dynamic and/or static fields bound. And in the definition of a ServiceMethod or implementation of SubServiceMethod it is a function returning an authorizer with the fields bound. It is probably not a big deal since the type has the information, but there is some disconnect when the types have different names Authorizer AuthorizerStaticFielsBound and so on but the prop is always authorizer. |
|
I totally agree. The naming could be a lot better, but this is one step in the right direction at least. |
Paulijuz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Im guessing this wont actually work
ablerto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥇
This PR removes getUser and useUser to move to new authers and authresults.
To remove getUser some services now do not have auth checks but the auth object has been set up ready for their refactor.