-
Notifications
You must be signed in to change notification settings - Fork 8
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
[Issue #1795]: Fix feature flag refresh bug #1863
Conversation
@@ -39,7 +39,8 @@ export function generateMetadata() { | |||
|
|||
export default async function Search({ searchParams }: ServerPageProps) { | |||
const ffManager = new FeatureFlagsManager(cookies()); | |||
if (!ffManager.isFeatureEnabled("showSearchV0")) { | |||
|
|||
if (ffManager.isFeatureDisabled("showSearchV0", searchParams)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch this to isFeatureDisabled
- reads a little better when the intent is to check if we should redirect to the NotFound
page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, tested locally.
@@ -6,6 +6,7 @@ import { NextRequest, NextResponse } from "next/server"; | |||
|
|||
import { CookiesStatic } from "js-cookie"; | |||
import { ReadonlyRequestCookies } from "next/dist/server/web/spec-extension/adapters/request-cookies"; | |||
import { ServerSideSearchParams } from "../types/searchRequestURLTypes"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of scope comment. the mixing of relative paths gave me pause. i created a ticket to address for the frontend: #1868 , so this is fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can take a look at making imports more consistent
Summary
Fixes #1795
Time to review: 5 min
Changes proposed
isFeatureEnabled
to have query params override the currently set cookie (which can become out of date)isFeatureDisabled
Context
The feature flag is set from a Next middleware function in
FeatureFlagsManager
. Unfortunately when this is set, it's not setting a value that's accessible to other code in the same request/response cycle (like the server component page) . The server component page has access tonext/headers
cookies
, but not from the ones set from the middleware function.So we can just pass the
SearchParams
that the server component has into theisFeatureEnabled
(andisFeatureDisabled
) for now. Acknowledging that there might be multiple ways to do this.Screen.Recording.2024-04-26.at.3.33.48.PM.mov