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

feat(unlock-app): NextAuth components #14025

Merged
merged 71 commits into from
Jun 20, 2024
Merged

feat(unlock-app): NextAuth components #14025

merged 71 commits into from
Jun 20, 2024

Conversation

SVell
Copy link
Contributor

@SVell SVell commented Jun 10, 2024

Description

This PR aims to bring new components required for NextAuth without any connecting logic

Issues

Fixes #
Refs #

Checklist:

  • 1 PR, 1 purpose: my Pull Request applies to a single purpose
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the docs to reflect my changes if applicable
  • I have added tests (and stories for frontend components) that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • If my code involves visual changes, I am adding applicable screenshots to this thread

Release Note Draft Snippet

screenshot

image

@cla-bot cla-bot bot added the cla-signed label Jun 10, 2024
@julien51
Copy link
Member

screenshots please?

@SVell SVell marked this pull request as ready for review June 12, 2024 15:38
@julien51
Copy link
Member

image

Instead of loading this in the "main" view/UI, can you please do that in a modal since this is auth related?

@@ -79,6 +81,7 @@
"typescript": "5.4.5",
"unified": "11.0.4",
"validator": "13.12.0",
"waas-sdk-ethers": "https://api.developer.coinbase.com/waas/consumer/1994648a1fa8a282f1c3ca917a0379f1f79fbb06/waas-sdk-ethers/waas-sdk-ethers-3.0.0.tgz",
Copy link
Member

Choose a reason for hiding this comment

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

I thought we said you will use our own library here?

import ConnectingWaas from '../interface/connect/ConnectingWaas'
import { AppLayout } from '../interface/layouts/AppLayout'

export const ConnectingContent = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Can you rather try to use the same layout as the "login" state?


useEffect(() => {
if (!timeoutRef.current) {
timeoutRef.current = setTimeout(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of the timeout here? I assume it should never happen, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If connecting takes to much time, we cancel the sign-in process here

Copy link
Member

Choose a reason for hiding this comment

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

Is that happening?

Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

That's a decent start! Please rebase and implement the changes!

},
]}
actions={
showConnectButton
Copy link
Member

Choose a reason for hiding this comment

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

You would not need to change any of these if you use the same layout for the ConnectingContent view, which I think is what you should do.

@@ -52,6 +52,8 @@ export const config = {
ethPassApiKey: 'sk_live_h0pHRAZ2E6WTkNIrXvEzbEQN39Ftrp1p',
walletConnectApiKey: '1535029cc7500ace23802e2e990c58d7', // https://cloud.walletconnect.com/app/project?uuid=7920be27-1e19-43a8-8f7d-cafbb00d4b80
googleMapsApiKey: 'AIzaSyDp0Y4yQn6WtYEFEgRZg52EiDSgLwxzVMA',
googleClientId: process.env.GOOGLE_CLIENT_ID, // https://console.cloud.google.com/apis/dashboard
Copy link
Member

Choose a reason for hiding this comment

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

This should porbably use the NEXT_PUBLIC_ prefix. Please confirm!

Copy link
Member

Choose a reason for hiding this comment

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

Or is it secret as well?

* @param {*} token
* @returns {Promise<*>}
*/
async getUserWaasUuid(emailAddress: string, provider: string, token: string) {
Copy link
Member

Choose a reason for hiding this comment

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

This should npt be in this file. Please add the API calls to openapi.yaml (inside of unlockjs) and then use the generated client.

Comment on lines 98 to 101
<span className="w-full max-w-lg text-base text-gray-700">
We are connecting to the Unlock Protocol, please be patient and do not
refresh the page.
</span>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<span className="w-full max-w-lg text-base text-gray-700">
We are connecting to the Unlock Protocol, please be patient and do not
refresh the page.
</span>
<span className="w-full max-w-lg text-base text-gray-700">
Signing in...
</span>

@@ -52,6 +52,9 @@ export const config = {
ethPassApiKey: 'sk_live_h0pHRAZ2E6WTkNIrXvEzbEQN39Ftrp1p',
walletConnectApiKey: '1535029cc7500ace23802e2e990c58d7', // https://cloud.walletconnect.com/app/project?uuid=7920be27-1e19-43a8-8f7d-cafbb00d4b80
googleMapsApiKey: 'AIzaSyDp0Y4yQn6WtYEFEgRZg52EiDSgLwxzVMA',
googleClientId: process.env.NEXT_PUBLIC_GOOGLE_CLIENT_ID, // https://console.cloud.google.com/apis/dashboard
googleClientSecret: process.env.NEXT_PUBLIC_GOOGLE_CLIENT_SECRET, // https://console.cloud.google.com/apis/dashboard
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
googleClientSecret: process.env.NEXT_PUBLIC_GOOGLE_CLIENT_SECRET, // https://console.cloud.google.com/apis/dashboard
googleClientSecret: process.env.GOOGLE_CLIENT_SECRET, // https://console.cloud.google.com/apis/dashboard

@SVell
Copy link
Contributor Author

SVell commented Jun 16, 2024

Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

One final change

@@ -82,7 +82,7 @@ jobs:
UNLOCK_APP_VERCEL_STAGING_VERCEL_ORG_ID: op://secrets/vercel/org-id
UNLOCK_APP_VERCEL_STAGING_VERCEL_TOKEN: op://secrets/vercel/deployment-token
UNLOCK_APP_VERCEL_STAGING_NEXT_PUBLIC_BASE64_WEDLOCKS_PUBLIC_KEY: op://secrets/wedlocks/public-key
UNLOCK_APP_VERCEL_STAGING_NEXTHAUTH_URL: 'https://unlock-protocol.com'
UNLOCK_APP_VERCEL_STAGING_NEXTAUTH_URL: 'https://unlock-protocol.com'
Copy link
Member

Choose a reason for hiding this comment

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

This would be more accurate?

Suggested change
UNLOCK_APP_VERCEL_STAGING_NEXTAUTH_URL: 'https://unlock-protocol.com'
UNLOCK_APP_VERCEL_STAGING_NEXTAUTH_URL: 'https://staging-app.unlock-protocol.com'

Copy link
Member

Choose a reason for hiding this comment

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

Also please add to the pull-request.yml and production workflows!

Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

Let's make sure we set the env correctly!

@@ -23,6 +23,7 @@ secrets:
NEXT_PUBLIC_ETHPASS_KEY:
required: false
env:
NEXTAUTH_URL: 'https://unlock-protocol.com'
Copy link
Member

Choose a reason for hiding this comment

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

This is porbabvly not correct?

It needs to be https://app.unlock-protocol.com or https://staging-app.unlock-protocol.com but please pass the value from the workflow that calls this action

Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

Still not correct

@@ -85,18 +21,9 @@ jobs:
UNLOCK_APP_VERCEL_STAGING_VERCEL_ORG_ID: op://secrets/vercel/org-id
UNLOCK_APP_VERCEL_STAGING_VERCEL_TOKEN: op://secrets/vercel/deployment-token
UNLOCK_APP_VERCEL_STAGING_NEXT_PUBLIC_BASE64_WEDLOCKS_PUBLIC_KEY: op://secrets/wedlocks/public-key
UNLOCK_APP_VERCEL_STAGING_GOOGLE_CLIENT_SECRET: op://secrets/google/staging-secret
UNLOCK_APP_VERCEL_STAGING_GOOGLE_CLIENT_SECRET: op://secrets/google/staging-secret

Copy link
Member

Choose a reason for hiding this comment

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

Why is there an extra space here?

- uses: ./.github/actions/vercel
with:
service: unlock-app
target-env: prod
env:
UNLOCK_APP_VERCEL_STAGING_NEXTAUTH_URL: 'https://app.unlock-protocol.com'
Copy link
Member

Choose a reason for hiding this comment

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

This CANNOT be STAGING. Staging is for Master.

Thi needs to be UNLOCK_APP_VERCEL_PROD_...

@julien51 julien51 merged commit d7d4e2e into master Jun 20, 2024
13 checks passed
@julien51 julien51 deleted the next-auth-front-start branch June 20, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants