Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[NAN-578] Implement Google Social Login/Signup #1872
[NAN-578] Implement Google Social Login/Signup #1872
Changes from 9 commits
cacdccd
8642139
68fec0b
9bd4f07
51538b2
818204e
386b6d0
fbd3c44
93ac937
c010b0b
6d7cf1a
a460604
0c5f6d5
95692ca
f155434
9659dc4
3dfd105
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
see my comment above. This is not a concern for the user and I think it would be better to crash the server at startup
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.
WorkOS is only required for Cloud so don't think it should crash here.
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.
what I mean is this is a configuration issue that would better detected as soon as possible (aka: when the app starts) rather than deferred to whenever a request is being received.
For sure for non-cloud setup this configuration can be ignored.
I prefer never to deployed a misconfigured app than getting a bug report by an end-user
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.
Yup, I gotcha, but who are we protecting here? Developers running the application locally? Cloud will have the necessary env variables.
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.
We protecting our future self
They will, until one day the infra changes and configuration is somehow forgotten or messed up.
In general trying to detect error as soon as possible is a good principle to follow.
Not a blocker though.
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 was the impression that
/ui
folder was for Generic reusable component, this one is really a one of. (my pov is that everything is related to ui anyway so it's either an unnecessary hierarchy level or it's specific for one scenario)