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

Update Clerk import in decoder #6685

Merged
merged 4 commits into from Oct 27, 2022

Conversation

stephenhandley
Copy link
Contributor

We ran into the following error when trying to use Clerk with Redwood.

api | 16:37:43 🚨 graphql-server Error building context. Error: Exception in getAuthenticationContext: Cannot find module './dist/instance'
api | Require stack:
api | - PROJECT_DIR/node_modules/@clerk/clerk-sdk-node/instance.js
api | - PROJECT_DI/node_modules/@redwoodjs/api/dist/auth/decoders/clerk.js
api | - PROJECT_DIR/node_modules/@redwoodjs/api/dist/auth/decoders/index.js
api | - PROJECT_DIR/node_modules/@redwoodjs/api/dist/auth/index.js
api | - PROJECT_DIR/node_modules/@redwoodjs/api/dist/index.js
api | - PROJECT_DIR/node_modules/@redwoodjs/graphql-server/dist/functions/graphql.js
api | - PROJECT_DIR/node_modules/@redwoodjs/graphql-server/dist/index.js
api | - PROJECT_DIR/api/dist/functions/graphql.

We had a dep of @clerk/clerk-sdk-node of 4.4.0

I believe what was at issue are the following lines in the decoder:
https://github.com/redwoodjs/redwood/blame/main/packages/api/src/auth/decoders/clerk.ts#L5-L7

const Clerk = require('@clerk/clerk-sdk-node/instance').default

const { users, base }: IClerk = new Clerk()

Clerk version 4.4.0 no longer has a dist/instance.js file instead there are dist/esm/instance.js and dist/cjs/instance.js files. they also now just export a singleton on the default. Changing the above lines to the following fixed things for us locally

const { users, base } = require('@clerk/clerk-sdk-node').default

@stephenhandley stephenhandley changed the title Update clerk import in decoder Update Clerk import in decoder Oct 13, 2022
@noire-munich
Copy link
Collaborator

@stephenhandley good to see you!
Many thanks for taking the time to make a PR on this, we shall look into it.

@noire-munich noire-munich removed their assignment Oct 13, 2022
@stephenhandley
Copy link
Contributor Author

Awesome thanks @noire-munich good to see you too!

@noire-munich noire-munich self-requested a review October 13, 2022 17:39
@Tobbe
Copy link
Member

Tobbe commented Oct 13, 2022

Just a heads up here 🙂 I merged #5985 about an hour ago. The clerk decoder now lives in https://github.com/redwoodjs/redwood/blob/main/packages/auth-providers-api/src/clerk/decoder.ts

@noire-munich noire-munich self-requested a review October 14, 2022 07:20
Copy link
Collaborator

@noire-munich noire-munich left a comment

Choose a reason for hiding this comment

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

Since decoupled auth has been merged the file has changed location, please check this comment
#6685 (comment)

@stephenhandley
Copy link
Contributor Author

@noire-munich @Tobbe ok just moved the changes to the new file path

@Tobbe
Copy link
Member

Tobbe commented Oct 14, 2022

We also have this PR that seems to be making a similar change. Which one should we go with? #6719

@stephenhandley
Copy link
Contributor Author

@Tobbe good thing from that PR missing from mine is all the bumped deps, however the implementation in the decoder looks a lot more brittle with the use of the instance paths.

@noire-munich noire-munich requested review from noire-munich and Tobbe and removed request for dthyresson October 15, 2022 09:42
@noire-munich
Copy link
Collaborator

@stephenhandley I'm holding my approval until either this PR or this one gets everything we feel we'd need.
I'd opt for your implementation of not using the instance paths, but @SokratisVidros being the founding engineer of Clerk I'm interested into knowing why he's using it.

@SokratisVidros
Copy link

@stephenhandley Thanks for submitting this.

@noire-munich The PRs are basically the same, fixing a backward incompatible change we made in @clerk/clerk-sdk-node.

You can merge either of them.

@MehdiSv
Copy link

MehdiSv commented Oct 21, 2022

@noire-munich Any update on this? We'd love to be able to use the latest version of Clerk with Redwood.

@alephao
Copy link
Contributor

alephao commented Oct 21, 2022

@noire-munich can we just approve this one then, it seems @SokratisVidros is fine with it and it doesn't use instance paths

@noire-munich
Copy link
Collaborator

@stephenhandley I've approved this, if you'd like to bump before we merge it would be great, otherwise we'll use the other PR for that.

@SokratisVidros
Copy link

Cool. I am closing mine.

@SokratisVidros
Copy link

Can we merge?

@Tobbe Tobbe added the release:fix This PR is a fix label Oct 27, 2022
@Tobbe
Copy link
Member

Tobbe commented Oct 27, 2022

I've confirmed that this fixes the require call for the decoder. Still seems like we have a problem with our Clerk implementation though. I'll try to merge this PR, and then I'll submit a new PR for fixing the new issue I found. (Navigating to a page that requires that you're logged in prints "Please wrap your auth provider with <ClerkLoaded>" in the console and just redirects you back to the home page. We do wrap our router in <ClerkLoaded>

@Tobbe Tobbe enabled auto-merge (squash) October 27, 2022 00:31
@jtoar jtoar disabled auto-merge October 27, 2022 00:37
@jtoar jtoar merged commit 204ad9a into redwoodjs:main Oct 27, 2022
@jtoar jtoar added this to the v4.0.0 milestone Oct 27, 2022
dac09 added a commit that referenced this pull request Oct 27, 2022
…o rc-service-caching

* 'rc-service-caching' of github.com:redwoodjs/redwood:
  chore: tweak release script for publishing from the next branch (#6745)
  Okta: Add packages to setup script (#6732)
  Azure setup auth: Install and import all needed packages (#6736)
  Setup auth: Update goTrue (#6733)
  Auth0 setup: Install correct packages (#6734)
  nhost auth: Add missing packages (#6742)
  Add missing packages to magicLink setup (#6741)
  supertokens setup auth: Add missing RW packages (#6744)
  Update Clerk import in decoder (#6685)
  Missing packages for Ethereum auth setup (#6740)
  supabase auth setup: Add missing rw packages (#6743)
  fix(Supabase): Set Supabase webPackage to 1.35.7 (#6739)
jtoar pushed a commit that referenced this pull request Nov 8, 2022
* update clerk import in decoder

* wrong import path

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
jtoar pushed a commit that referenced this pull request Nov 8, 2022
* update clerk import in decoder

* wrong import path

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
@jtoar jtoar modified the milestones: v4.0.0, next-release-patch, v3.3.2 Nov 10, 2022
jtoar pushed a commit that referenced this pull request Nov 10, 2022
* update clerk import in decoder

* wrong import path

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants