-
Notifications
You must be signed in to change notification settings - Fork 53
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
Replace organization query with whoami query #6109
Replace organization query with whoami query #6109
Conversation
phone | ||
deviceTypes { | ||
whoami { |
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.
The organization is now nested in whoami, no fields were actually changed.
frontend/src/app/Settings/types.d.ts
Outdated
@@ -150,3 +150,8 @@ type OrganizationType = | |||
| "camp" | |||
| "lab" | |||
| "other"; | |||
|
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.
Could we move this type declaration out of the global import and put it in a named .ts
file? The Typescript team discourages this pattern since there might be compilation issues without a corresponding JS file.
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.
I didn't know that the d.ts use was discouraged in typescript because the context of the type can be messed up. Sounds like these files are more like a result of typescript being compile into JS.
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.
Yeah so based on the reading I did, the .d.ts
files are there for libraries where typescript and javascript files are sitting side by side. In cases where JS files are necessary, adding the .d.ts
files allows you to still have access to the relevant types while not requiring those files to be typescript files.
Based on what I found this pattern isn't as big an issue for applications since the main antipattern is global access when libraries are imported (which would make those variables available at the global namespace even outside the library) but I think it's still worth it to make it explicit where these new types are coming from just so there isn't any confusion / to avoid possible namespace collision issues.
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 overall! left one suggestion
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.
Tested in dev and worked as expected. I just left two comments. Thanks! 💯
frontend/src/app/Settings/types.d.ts
Outdated
@@ -150,3 +150,8 @@ type OrganizationType = | |||
| "camp" | |||
| "lab" | |||
| "other"; | |||
|
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.
I didn't know that the d.ts use was discouraged in typescript because the context of the type can be messed up. Sounds like these files are more like a result of typescript being compile into JS.
Kudos, SonarCloud Quality Gate passed! |
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!
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! thanks for switching the file type around :)
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.
changes on dev3 looked good!
BACKEND PULL REQUEST
Related Issue
Changes Proposed
organization
query withwhoami
operations.graphql
Additional Information
organization
query to make it fetch by id rather than user contextTesting