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

AuthProvider: return signup() output from auth client #1992

Merged
merged 3 commits into from
Mar 16, 2021

Conversation

jvanbaarsen
Copy link
Contributor

When you signup via Supabase you get all the meta data back in the
return of the signup call. We now eat all that output instead of
giving it back to the user.

I noticed that we already do something similar to Sign In, so I
basically took that concept and applied that for signUp as well.

@dthyresson Do you think we break people's app by not returning reauthenticate()?

When you signup via Supabase you get all the meta data back in the
return of the `signup` call. We now eat all that output instead of
giving it back to the user.

I noticed that we already do something similar to Sign In, so I
basically took that concept and applied that for signUp as well.
@github-actions
Copy link

github-actions bot commented Mar 12, 2021

📦 PR Packages

Click to Show Package Download Links

https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/create-redwood-app-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-api-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-api-server-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-auth-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-cli-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-core-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-dev-server-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-eslint-config-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-eslint-plugin-redwood-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-forms-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-internal-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-prerender-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-router-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-structure-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-testing-0.27.1-95a2319.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1992/redwoodjs-web-0.27.1-95a2319.tgz

Install this PR by running yarn rw upgrade --pr 1992:0.27.1-95a2319

@dthyresson dthyresson self-requested a review March 14, 2021 12:14
Copy link
Contributor

@dthyresson dthyresson left a comment

Choose a reason for hiding this comment

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

@jvanbaarsen I think this is fine. @Tobbe what do you think?

The playground auth should be updated with this PR to test too.

In fact there are a few Supabase updates that need to be done to get up to current including redirectTo and oauth scope support.

@Tobbe
Copy link
Member

Tobbe commented Mar 14, 2021

I think this is the correct behavior.

But, this is where semver is difficult. Is this a breaking change or not? Theoretically it is, signUp still returns a promise, But previously it would always resolve to undefined, now it'll resolve to something else. How likely is it that someone has code that depends on the promise resoling to undefined? Like 0.0001% risk...

@jvanbaarsen
Copy link
Contributor Author

@Tobbe My feel is that this is not really a "breaking" change. We can mention it in the release notes so the 0.00001% of people run into this are aware of the problem.

@jvanbaarsen
Copy link
Contributor Author

In the auth example I'm wondering how https://github.com/redwoodjs/playground-auth/blob/main/web/src/components/Supabase/Supabase.js#L81 could have worked. Since Supabase eats up the error that they throw and add it to the return object, so an error is never thrown. Or was that the old integration they used?

@Tobbe
Copy link
Member

Tobbe commented Mar 16, 2021

@jvanbaarsen Yeah, you might be right. They changed that back in Oct of last year. https://supabase.io/blog/2020/10/30/improved-dx

@Tobbe Tobbe changed the title Return signup Output AuthProvider: return signup() output from auth client Mar 16, 2021
@thedavidprice
Copy link
Contributor

@Tobbe @jvanbaarsen @dthyresson

  • is this one ready for final review (or ready to merge?) Seems like there are a couple potential next steps required.
  • could I get a one-sentence description of the behavior change associate with this PR --> agreed this is not "breaking", but I would like to highlight the change with a separate bullet point

Thanks again!

@Tobbe
Copy link
Member

Tobbe commented Mar 16, 2021

image

I did that with the intent to merge as soon as all checks had completed, but then I had to go pick up the kids. I'll try again.

@Tobbe
Copy link
Member

Tobbe commented Mar 16, 2021

could I get a one-sentence description of the behavior change associate with this PR

RW's signUp from useAuth returns a promise that will now resolve with the return value of the underlying auth client's sign up method. Previously that promise always resolved with undefined

Sorry. That's two sentences.

@Tobbe Tobbe merged commit 89e2a8d into redwoodjs:main Mar 16, 2021
@thedavidprice thedavidprice added this to the next release milestone Mar 16, 2021
@jvanbaarsen jvanbaarsen deleted the signupOutput branch March 17, 2021 12:00
@jvanbaarsen
Copy link
Contributor Author

@Tobbe Thanks for taking this across the line!

dac09 added a commit to dac09/redwood that referenced this pull request Mar 22, 2021
…r-rwt-link

* 'main' of github.com:redwoodjs/redwood: (26 commits)
  Router: Fix auth (redwoodjs#2038)
  Steps towards a11y for Redwood Router (redwoodjs#1817)
  Router: <Set wrap={}> (redwoodjs#1898)
  Pass event and context into getCurrentUser (redwoodjs#1908)
  Implement Redwood API side Logger (redwoodjs#1937)
  Fixed path on windows to allow for pages under subdirectories (redwoodjs#2022)
  Add experimental ESBuild for api side (redwoodjs#1948)
  Upgrade to Prisma 2.19.0 (Prisma Migrate GA) (redwoodjs#2021)
  Fix lint breaking when deleting a side (redwoodjs#2017)
  Refactor: Converted Prisma.ts to js (redwoodjs#1958)
  Fix issue with verify email redirect using Auth0 (redwoodjs#1990)
  add GitHub Action CodeQL Analysis (redwoodjs#1951)
  fix: correct var name for grabbing user config (redwoodjs#2002)
  Create functions to fs calls (redwoodjs#2007)
  Return signup Output (redwoodjs#1992)
  Add makeExecSchema options (redwoodjs#1964)
  upgrade gotrue-js to 0.9.29 (redwoodjs#2011)
  Azure Active Directory Auth: Adding try-catch block on callback to capture empty key (redwoodjs#2010)
  withCellHOC: Fix TS error (redwoodjs#1967)
  Update error message in tasks/publish-local to point to tasks/run-local-npm when Verdaccio isn't running (redwoodjs#2004)
  ...
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.

4 participants