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

fix(clerk-sdk-node): load config on first use #1230

Merged
merged 2 commits into from
Jun 12, 2023
Merged

Conversation

dimkl
Copy link
Member

@dimkl dimkl commented May 23, 2023

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Packages affected

  • @clerk/clerk-js
  • @clerk/clerk-react
  • @clerk/nextjs
  • @clerk/remix
  • @clerk/types
  • @clerk/themes
  • @clerk/localizations
  • @clerk/clerk-expo
  • @clerk/backend
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/fastify
  • @clerk/chrome-extension
  • gatsby-plugin-clerk
  • build/tooling/chore

Description

  • npm test runs as expected.
  • npm run build runs as expected.

@dimkl dimkl self-assigned this May 23, 2023
@dimkl dimkl requested a review from nikosdouvlis as a code owner May 23, 2023 14:00
@dimkl dimkl closed this May 23, 2023
Copy link

@jit-ci jit-ci bot left a comment

Choose a reason for hiding this comment

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

❌ The following Jit checks failed to run:

  • software-component-analysis-js

#jit_bypass_commit in this PR to bypass, Jit Admin privileges required.

More info in the Jit platform.

@dimkl dimkl reopened this May 23, 2023
@dimkl dimkl force-pushed the js-323-dotenv-imports branch 5 times, most recently from 1451f8d to eb6386a Compare May 26, 2023 18:07
Copy link
Member

@anagstef anagstef left a comment

Choose a reason for hiding this comment

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

🧹 🧹 🧹

@dimkl dimkl force-pushed the js-323-dotenv-imports branch 2 times, most recently from eb4b4a5 to b7d49f4 Compare May 29, 2023 14:43
@SokratisVidros
Copy link
Contributor

@dimkl Can you please elaborate on the issue that this change addresses?

@nikosdouvlis
Copy link
Member

@dimkl Can you please elaborate on the issue that this change addresses?

Currently in Clerk Node, the following code doesn't run because Clerk doesn't read any of the keys even though they are clearly imported, this leads to a bad DX and reports that Clerk doesn't work correctly.

import { ClerkExpressWithAuth } from "@clerk/clerk-sdk-node";
import express from "express";

require('dotenv').config()

const port = process.env.PORT || 3000;

const app = express();
// Use the lax middleware that returns an empty auth object when unauthenticated
app.get(
  "/protected-endpoint",
  ClerkExpressWithAuth({
    // ...options
  }),
  (req, res) => {
    res.json(req.auth);
  }
);

app.listen(port, () => {
  console.log(`Example app listening at http://localhost:${port}`);
});

Notice that since there is no underlying framework to load the env before the Clerk imports are evaluated, the order of the imports and the dotenv.config invocation is significant here, it's very likely for Clerk to be initialized before env.process is.

We need to read the current env value when needed, not just once during initialization.

@changeset-bot
Copy link

changeset-bot bot commented Jun 12, 2023

🦋 Changeset detected

Latest commit: 63eb73b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@clerk/clerk-sdk-node Patch
gatsby-plugin-clerk Patch
@clerk/nextjs Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dimkl dimkl force-pushed the js-323-dotenv-imports branch 3 times, most recently from afbb951 to 4c4626c Compare June 12, 2023 14:52
First usage of clerkClient can be either from accessing a method
from clerkClient or from using the middleware exposed from the
package which are using the clerkClient exposed by package.
@dimkl dimkl merged commit eff4e45 into main Jun 12, 2023
9 checks passed
@dimkl dimkl deleted the js-323-dotenv-imports branch June 12, 2023 16:26
@clerk-cookie clerk-cookie mentioned this pull request Jun 12, 2023
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

4 participants