chore!: deprecate `reason` for `ArcjetAllowDecision` #2715

e-moran · 2025-01-02T16:49:33Z

This PR makes reason optional for ArcjetAllowDecision and deprecates it.

We are making this change because the rule that is accessible through the reason field is not obvious to our users and has the potential to cause confusion.

Instead we recommend iterating through the ruleResults list to find rule you are looking for.

trunk-io · 2025-01-02T16:49:36Z

Merging to main in this repository is managed by Trunk.

To merge this pull request, check the box to the left or comment /trunk merge below.

davidmytton · 2025-01-02T17:05:31Z

Can you add the migration example code to this PR description please? It should be documented in the changelog as well, but it'll be helpful to have it in this PR in case someone comes across it.

blaine-arcjet · 2025-01-02T23:00:40Z

decorate/index.ts

@@ -208,7 +208,7 @@ export function setRateLimitHeaders(
      .sort(sortByLowestMax)
      .map(toPolicyString)
      .join(", ");
-  } else {
+  } else if (typeof decision.reason !== "undefined") {


Why doesn't isRateLimitReason just taking an optional reason?

examples/nextjs-bot-categories/app/api/arcjet/route.ts

examples/nextjs-clerk-rate-limit/app/api/arcjet/route.ts

examples/nextjs-openai/app/api/chat/route.ts

protocol/test/convert.test.ts

blaine-arcjet · 2025-01-02T23:04:51Z

protocol/index.ts

@@ -591,7 +591,7 @@ export abstract class ArcjetDecision {
  ip: ArcjetIpDetails;

  abstract conclusion: ArcjetConclusion;
-  abstract reason: ArcjetReason;
+  abstract reason: ArcjetReason | undefined;


This stinks. We might want to remove these abstract fields.

They could be useful if someone wants to type-check on a generic ArcjetDecision to see if it has a reason or not.

I need to double check, but I don't think this needs to be abstract to do that.

blaine-arcjet · 2025-01-02T23:04:59Z

protocol/index.ts

+export type ArcjetDecisionWithReason = ArcjetDecision & {
+  reason: NonNullable<ArcjetDecision["reason"]>;
+};


I don't like this.

Do you not like its existence, or just how its defined?
I could change it to just ArcjetDecision & { reason: ArcjetReason}, or else we'd have to use that definition in every function that requires a reason.

I don't think we need to provide it because they could write their own intersection. Right?

Yeah they could! I've removed it

blaine-arcjet · 2025-01-09T21:37:14Z

examples/nextjs-clerk-rate-limit/app/api/arcjet/route.ts

+    const policies = new Map<number, number>();
+    for (const reason of rateLimitReasons) {
+      if (policies.has(reason.max)) {
+        console.error(
+          "Invalid rate limit policy—two policies should not share the same limit",
+        );
+      }
+
+      if (
+        typeof reason.max !== "number" ||
+        typeof reason.window !== "number" ||
+        typeof reason.remaining !== "number" ||
+        typeof reason.reset !== "number"
+      ) {
+        console.error(format("Invalid rate limit encountered: %o", reason));
+      }
+
+      policies.set(reason.max, reason.window);
+    }


Policies isn't doing anything? Did you want to break out of the logic or something?

Its used to print an error if two rate limit policies share the same limit, like we do in decorate. The difference is that decorate returns an error in this case. We could respond with a 500 here to have the same behavior. What do you think?

Another option would be to detect invalid configs earlier, such as when initializing Arcjet. But that would be a much larger change.

blaine-arcjet · 2025-01-09T21:37:58Z

examples/nextjs-clerk-rate-limit/package.json

@@ -10,6 +10,7 @@
  },
  "dependencies": {
    "@arcjet/next": "file:../../arcjet-next",
+    "@arcjet/sprintf": "file:../../sprintf",


Might not be needed if you don't keep the policies stuff

blaine-arcjet · 2025-01-09T21:46:21Z

examples/nextjs-openai/app/api/chat/route.ts

+        console.error(format("Invalid rate limit encountered: %o", reason));
+      }
+
+      policies.set(reason.max, reason.window);


blaine-arcjet · 2025-01-09T21:46:43Z

examples/nextjs-openai/package.json

@@ -11,6 +11,7 @@
  "dependencies": {
    "@ai-sdk/openai": "^1.0.8",
    "@arcjet/next": "file:../../arcjet-next",
+    "@arcjet/sprintf": "file:../../sprintf",


blaine-arcjet · 2025-01-09T21:47:28Z

protocol/index.ts

+export type ArcjetDecisionWithReason = ArcjetDecision & {
+  reason: NonNullable<ArcjetDecision["reason"]>;
+};


I don't think we need to provide it because they could write their own intersection. Right?

blaine-arcjet · 2025-01-09T21:50:08Z

examples/nextjs-bot-categories/app/api/arcjet/route.ts

-        { status: 403, headers },
-      );
-    }
+  const allowedBots = decision.results.reduce<string[]>(reduceAllowedBots, []);


I noticed you are still using map/filter in the other examples (for rate limit). We should keep it uniform. The reason to use a reduce is to avoid creating extra arrays, but the implementation of the reduction function is creating a new array on each iteration which is worse than a single filter then a map.

I've changed this to a reduce. This removes checking that a limit isn't duplicated which is discussed in another comment though.

blaine-arcjet · 2025-01-09T21:50:50Z

protocol/client.ts

@@ -127,7 +128,7 @@ export function createClient(options: ClientOptions): Client {
    report(
      context: ArcjetContext,
      details: ArcjetRequestDetails,
-      decision: ArcjetDecision,
+      decision: ArcjetDecisionWithReason,


It doesn't need to have a reason, right?

blaine-arcjet · 2025-01-09T21:51:29Z

protocol/index.ts

What happened with also deprecating reason on ArcjetErrorDecision?

blaine-arcjet · 2025-01-09T21:51:52Z

protocol/index.ts

 export class ArcjetAllowDecision extends ArcjetDecision {
  conclusion = "ALLOW" as const;
-  reason: ArcjetReason;
+  /** @deprecated */


needs a message

blaine-arcjet · 2025-01-09T21:52:27Z

protocol/index.ts

@@ -591,7 +591,7 @@ export abstract class ArcjetDecision {
  ip: ArcjetIpDetails;

  abstract conclusion: ArcjetConclusion;
-  abstract reason: ArcjetReason;
+  abstract reason: ArcjetReason | undefined;


I need to double check, but I don't think this needs to be abstract to do that.

…e-reason-for-allow

@deprecated

…2855) While reviewing #2715 locally, I noticed that `/** @deprecated */` wasn't being marked in my editor. This is because `/** */` is just a block comment and not a doc comment. Doc comments need to be in the form as I've changed this PR. Additionally, deprecating the top-level export didn't seem to mark anything in the editor, so each field needed to be marked as deprecated. Furthermore, overriding the type as `ArcjetEnum<>` seemed to lose the doc comments. I also removed the exports locally and made everything compile once they are removed. They still exist for now but I want the removal to be seamless.

blaine-arcjet · 2025-01-15T17:14:50Z

Converting to draft while we discuss this further.

e-moran requested a review from a team as a code owner January 2, 2025 16:49

e-moran added 2 commits January 2, 2025 17:48

chore!: deprecate reason for ArcjetAllowDecision

2451659

fix: examples

8b78916

e-moran force-pushed the eoin/deprecate-reason-for-allow branch from 252d92f to 8b78916 Compare January 2, 2025 17:49

e-moran changed the title ~~deprecate!: deprecate reason for ArcjetAllowDecision~~ chore!: deprecate reason for ArcjetAllowDecision Jan 2, 2025

blaine-arcjet requested changes Jan 2, 2025

View reviewed changes

e-moran added 3 commits January 8, 2025 17:37

update examples

3173d91

fix chatuserid

e3fc641

remove arrow functions

5b25ae9

blaine-arcjet requested changes Jan 9, 2025

View reviewed changes

e-moran added 4 commits January 10, 2025 14:33

pr comments

312655b

Merge branch 'main' of github.com:arcjet/arcjet-js into eoin/deprecat…

0caefa4

…e-reason-for-allow

remove udep

a6dcec4

fix error handling

af75332

blaine-arcjet mentioned this pull request Jan 14, 2025

fix(protocol)!: Improve deprecation message on enum-like field usage #2855

Merged

blaine-arcjet mentioned this pull request Jan 14, 2025

WIP cleanup reason deprecation #2857

Merged

WIP cleanup reason deprecation (#2857)

9fc8d23

blaine-arcjet marked this pull request as draft January 15, 2025 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore!: deprecate `reason` for `ArcjetAllowDecision` #2715

chore!: deprecate `reason` for `ArcjetAllowDecision` #2715

e-moran commented Jan 2, 2025

trunk-io bot commented Jan 2, 2025

davidmytton commented Jan 2, 2025

blaine-arcjet Jan 2, 2025

blaine-arcjet Jan 2, 2025

e-moran Jan 8, 2025

blaine-arcjet Jan 9, 2025

blaine-arcjet Jan 2, 2025

e-moran Jan 8, 2025

blaine-arcjet Jan 9, 2025

e-moran Jan 10, 2025

blaine-arcjet Jan 9, 2025

e-moran Jan 10, 2025 •

edited

Loading

blaine-arcjet Jan 9, 2025

blaine-arcjet Jan 9, 2025

blaine-arcjet Jan 9, 2025

blaine-arcjet Jan 9, 2025

blaine-arcjet Jan 9, 2025

e-moran Jan 10, 2025

blaine-arcjet Jan 9, 2025

blaine-arcjet Jan 9, 2025

blaine-arcjet Jan 9, 2025

blaine-arcjet Jan 9, 2025

blaine-arcjet commented Jan 15, 2025

chore!: deprecate reason for ArcjetAllowDecision #2715

Are you sure you want to change the base?

chore!: deprecate reason for ArcjetAllowDecision #2715

Conversation

e-moran commented Jan 2, 2025

trunk-io bot commented Jan 2, 2025

davidmytton commented Jan 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-moran Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaine-arcjet commented Jan 15, 2025

chore!: deprecate `reason` for `ArcjetAllowDecision` #2715

chore!: deprecate `reason` for `ArcjetAllowDecision` #2715

e-moran Jan 10, 2025 •

edited

Loading