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

Support experimental_ignoreInterstitial option and use it for api routes #1429

Closed
wants to merge 4 commits into from

Conversation

dimkl
Copy link
Contributor

@dimkl dimkl commented Jun 28, 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.

Handle interstitial as signed-out for API routes.
Example usages

// 1. @clerk/backend
import { clerkClient } from "@clerk/backend";

clerkClient.authenticateRequest({
  experimental_ignoreInterstitial: true
});

// 2. @clerk/clerk-sdk-node with optional authentication for specific route

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

const experimentalIsApiRoute = (url: URL) => url.pathname == '/public';
router.use(ClerkExpressWithAuth({ experimental_isApiRoute }));

// 3. @clerk/clerk-sdk-node with required authentication for all routes

import { ClerkExpressRequireAuth } from '@clerk/clerk-sdk-node';

router.use(ClerkExpressRequireAuth({ experimental_isApiRoute: true }));

@dimkl dimkl self-assigned this Jun 28, 2023
@changeset-bot
Copy link

changeset-bot bot commented Jun 28, 2023

🦋 Changeset detected

Latest commit: 776c0a6

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

This PR includes changesets to release 6 packages
Name Type
@clerk/clerk-sdk-node Minor
@clerk/backend Minor
gatsby-plugin-clerk Patch
@clerk/nextjs Patch
@clerk/fastify Patch
@clerk/remix 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

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.

✅ Great news! Jit hasn't found any security issues in your PR. Good Job! 🏆

/**
* @experimental
*/
signInUrl?: string;
isApiRoute?: boolean;
Copy link
Contributor

@SokratisVidros SokratisVidros Jun 29, 2023

Choose a reason for hiding this comment

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

@dimkl I think that we should make this more generic and call it ignoreInterstitial, which is either a boolean or a function boolean | ((url: URL | string) => string);. That way, we can cover more use cases such as assets, etc...

The apiRoutes option we added in @clerk/nextjs should also be able to leverage it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't add the function in type since the url of the request is not available in the @clerk/backend package.
We can do that after the addition of the request parameter (see: #1393)

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. @nikosdouvlis how far are we from merging #1393 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SokratisVidros, i think we can continue with the boolean type and enhance it ( boolean | ((url: URL | string) => boolean);) when request parameter PR is merged.

Copy link
Member

Choose a reason for hiding this comment

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

The authenticateRequest PR should be merged today, but lets do what @dimkl suggested.

Dimitri, can we add a JSDOC comment documenting what the prop does?

I'm not a fan of the ignoreIInterstitial name as it feels like an escape hatch and nothing a non-advanced Clerk user would understand. That being said, I don't have any better alternatives at the moment.

@dimkl dimkl changed the title Js 501 detect api routes Support ignoreInterstitial option and use it for api routes Jun 29, 2023
@dimkl dimkl force-pushed the js-501-detect-api-routes branch 2 times, most recently from 5ab58bf to f6e3882 Compare June 29, 2023 09:38
packages/sdk-node/src/types.ts Outdated Show resolved Hide resolved
packages/backend/src/tokens/request.ts Outdated Show resolved Hide resolved
@dimkl dimkl force-pushed the js-501-detect-api-routes branch 2 times, most recently from ac974b6 to 723bdbd Compare June 29, 2023 10:30
@dimkl
Copy link
Contributor Author

dimkl commented Jun 29, 2023

@SokratisVidros , @nikosdouvlis comments have been addressed. PTAL.

@dimkl dimkl changed the title Support ignoreInterstitial option and use it for api routes Support experimental_ignoreInterstitial option and use it for api routes Jun 29, 2023
@dimkl dimkl force-pushed the js-501-detect-api-routes branch 4 times, most recently from 06d9ad8 to 383c628 Compare June 29, 2023 22:33
@dimkl
Copy link
Contributor Author

dimkl commented Jun 29, 2023

!snapshot

@clerk-cookie
Copy link
Collaborator

Hey @dimkl - the snapshot version command generated the following package versions:

Package Version
@clerk/backend 0.24.0-snapshot.383c628
@clerk/chrome-extension 0.3.19-snapshot.383c628
@clerk/clerk-js 4.51.0
eslint-config-custom 0.3.0
@clerk/clerk-expo 0.18.10-snapshot.383c628
@clerk/fastify 0.5.6-snapshot.383c628
gatsby-plugin-clerk 4.3.19-snapshot.383c628
@clerk/localizations 1.22.0
@clerk/nextjs 4.21.13-snapshot.383c628
@clerk/clerk-react 4.21.1-snapshot.383c628
@clerk/remix 2.6.16-snapshot.383c628
@clerk/clerk-sdk-node 4.11.0-snapshot.383c628
@clerk/shared 0.19.1
@clerk/themes 1.7.5
@clerk/types 3.46.1

Tip: use the snippet copy button below to quickly install the required packages.

# @clerk/backend
npm i @clerk/backend@0.24.0-snapshot.383c628
# @clerk/chrome-extension
npm i @clerk/chrome-extension@0.3.19-snapshot.383c628
# @clerk/clerk-js
npm i @clerk/clerk-js@4.51.0
# eslint-config-custom
npm i eslint-config-custom@0.3.0
# @clerk/clerk-expo
npm i @clerk/clerk-expo@0.18.10-snapshot.383c628
# @clerk/fastify
npm i @clerk/fastify@0.5.6-snapshot.383c628
# gatsby-plugin-clerk
npm i gatsby-plugin-clerk@4.3.19-snapshot.383c628
# @clerk/localizations
npm i @clerk/localizations@1.22.0
# @clerk/nextjs
npm i @clerk/nextjs@4.21.13-snapshot.383c628
# @clerk/clerk-react
npm i @clerk/clerk-react@4.21.1-snapshot.383c628
# @clerk/remix
npm i @clerk/remix@2.6.16-snapshot.383c628
# @clerk/clerk-sdk-node
npm i @clerk/clerk-sdk-node@4.11.0-snapshot.383c628
# @clerk/shared
npm i @clerk/shared@0.19.1
# @clerk/themes
npm i @clerk/themes@1.7.5
# @clerk/types
npm i @clerk/types@3.46.1

/**
* @experimental
*/
experimental_isApiRoute?: BooleanOrURLFnToBoolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Why not experimental_ignoreInterstitial?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that frameworks using the @clerk/backend would have the option to define their API routes (that's why I have set it as experimental_isApiRoute ) and that we could use it internally to whatever we want eg:

  • ignore interstitial
  • avoid redirecting to sign-in (case of NextJS)
    Mainly I have used the same logic we have for NextJS and applied it to sdk-node.
    Should I rename it to experimental_ignoreInterstitial or not?
    cc: @nikosdouvlis

@nikosdouvlis
Copy link
Member

Closing in favor of the solutions we're discussing at https://www.notion.so/clerkdev/Interstitial-public-Api-Pages-10369bee0edf47f3b410e45f2723f12c

@dimkl dimkl deleted the js-501-detect-api-routes branch March 29, 2024 10:40
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