feat: Initial Login flow #42
Conversation
Codecov Report
@@ Coverage Diff @@
## main #42 +/- ##
==========================================
+ Coverage 64.70% 71.10% +6.39%
==========================================
Files 19 59 +40
Lines 153 2308 +2155
Branches 9 30 +21
==========================================
+ Hits 99 1641 +1542
- Misses 50 521 +471
- Partials 4 146 +142
Continue to review full report at Codecov.
|
|
|
||
| type PasswordFieldProps = Omit<TextFieldProps, "InputProps" | "type"> | ||
|
|
||
| export const PasswordField: React.FC<PasswordFieldProps> = ({ variant = "outlined", ...rest }) => { |
There was a problem hiding this comment.
Why is this called PasswordField, but the other named FormTextField?
There was a problem hiding this comment.
This is brought in mostly as-is from v1, but there's actually a has-a relationship here.
FormTextField contains either:
TextFieldPasswordField
In the v1 coder/m codebase - we used the prefix Form* to denote that the controller is meant to be bound to a formik form. That means that it exposes a couple of properties:
form: a formik formformFieldName: the field name to bind too
With that, the Form* components handle binding to Formik:
onChange(calls into formik)onBlur(calls into formik)error(sets appropriate property on control based on form state + field)helperText(set to error text if there is an error, otherwise helper text if provided)id(set to form field name)
So essentially FormTextField is a convenience component that helps bind to formik - instead of having to bind to 5 different props to get proper form behavior (and potentially doing it inconsistently) we only have to bind to two props - form and formFieldName, and get common behavior across our forms.
And it happens that FormTextField can either use a TextField or PasswordField under the hood, depending on the isPassword prop. TextField and PasswordField don't know anything at all about formik - they're just 'dumb' input components.
In the v1/coder/m codebase, we actually have a bunch of these Form* components, including:
FormCheckboxFormRadioGroupFormField
...and we've talked about a few others, likeFormToggleSwitch
Hope that helps clarify - let me know if you have other questions
There was a problem hiding this comment.
Ahh this is very helpful context. Thanks Bryan!
| * CoderIcon represents the cloud with brackets Coder brand icon. It does not | ||
| * contain additional aspects, like the word 'Coder'. | ||
| */ | ||
| export const CoderIcon = (props: SvgIconProps): JSX.Element => ( |
There was a problem hiding this comment.
I know we didn't in the other repo, but it might be wise to consider a default export idiom for this codebase.
CoderIcon.tsx should default export CoderIcon from my POV.
There was a problem hiding this comment.
This is actually kind of contentious in the TypeScript world - there's a couple of quirks with default exports.
- The refactoring tools in VSCode / TypeScript language server don't work as well with default exports
- In some compilation modes, the default export is kind of awkward (it literally exports a named module 'export', which can cause problems with some tools). NextJS handles this OK, but if we use a tool with a Babel pipeline like storybook, we can sometimes run into weird issues.
There's a bit more info in these links:
- https://stackoverflow.com/questions/33305954/typescript-export-vs-default-export
- https://basarat.gitbook.io/typescript/main-1/defaultisbad
That being said, it seems to work fine for our pages/ 😄 And I think the icons is a reasonable use-case for this, I agree with you conceptually that it makes sense for CoderIcon.tsx. We can try it out if you want.
There was a problem hiding this comment.
With the context you provided here, I'll trust the direction you're taking here! Let's leave it as-is for now, everything seems to point away from the defaults hahah
| import { FullScreenLoader } from "../Loader/FullScreenLoader" | ||
| import * as API from "./../../api" | ||
|
|
||
| export const AuthenticatedRouter: React.FC = ({ children }) => { |
There was a problem hiding this comment.
Should we do this in _app.tsx instead? Seems quite standard.
There was a problem hiding this comment.
As it is currently implemented - this is a case where it actually wouldn't make sense to include this in _app.tsx - if we did, the /login route would get in an infinite loop. (The _app.tsx would fetch the current user, fail, redirect to login.... which would render _app.tsx first, fail, redirect to login... forever 😄 )
But I think this an example of a case where something like an <AppPage /> can help us (from the comment in the other PR here: #33 (comment)) - every <AppPage /> should include this.
However, if we removed the login.tsx, and just implemented that full flow in _app.tsx - it could work. My concern is that the _app.tsx can grow really fast and it could also add complexity when we add other login flows besides built-in auth (ie, OIDC)
| useEffect(() => { | ||
| const asyncFn = async () => { | ||
| try { | ||
| await API.User.current() |
There was a problem hiding this comment.
This might be a good opportunity to introduce SWC. It gives us a lot of beeeeautiful inherit benefits.
Primarily in periodically refreshing to ensure state is accurate with the backend.
There was a problem hiding this comment.
Sure, I'm open to exploring it!
Are you OK if we track this outside of the PR? And do you think it is necessary for black triangle, or something we can defer until after?
Opened this issue to track, and I marked it as a MVP for the time being: #43 but let me know if its something you'd prefer to do in this PR (or outside the PR, but for black triangle, and I'll update the milestone)
There was a problem hiding this comment.
Well, the reason I recommend we use it is actually for simplicity. Although there's little familiarity, I'm not sure there needs to be: https://github.com/coder/link/blob/main/site/src/contexts/UserContext.tsx#L35
Just makes it really simple, and removes the requirement of abstracting a fetch at all.
This is up to you as to whether we use it or not. It does handle all the errors n' such nicely though, so could be cool!
There was a problem hiding this comment.
Cool, I'll take a look as part of this PR then and try it out! Thanks for the link to the usage in link - helpful
There was a problem hiding this comment.
I tried it out and it actually makes things more complicated (for login, at least). It isn't really meant to POST or take body arguments out of the box.
Here's a GH issue describing setting up a login flow:
vercel/swr#553 (comment)
I'll revisit for some of the other places, though, where might be able to take better advantage of the error handling / refresh behavior.
There was a problem hiding this comment.
Ahh I mean mostly for fetching. Much less useful for any other type of request.
|
Thanks for reviewing, @kylecarbs ! |
This just implements a basic sign-in flow, using the new endpoints in #29 :
This brings over several dependencies that are necessary:
formikyepPorts over some v1 code to bootstrap it:
FormTextFieldPasswordFieldCoderIconTODO:
/loginif not authenticatedlink-style flow/userAPI to useSWR-Fixes #37
Fixes #43
This does not implement it navbar integration (importantly - there is no way to sign out yet, unless you manually delete your
session_token). I'll do that in the next PR - figured this was big enough to get reviewed.