-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat: add support for building Next.JS apps that use suspending flags. #1157
Open
beeme1mr
wants to merge
3
commits into
main
Choose a base branch
from
improve-react-suspense
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
|
||
|
||
const context = 'Components using OpenFeature must be wrapped with an <OpenFeatureProvider>.'; | ||
const tip = 'If you are seeing this in a test, see: https://openfeature.dev/docs/reference/technologies/client/web/react#testing'; | ||
|
||
export class MissingContextError extends Error { | ||
constructor(reason: string) { | ||
super(`${reason}: ${context} ${tip}`); | ||
this.name = 'MissingContextError'; | ||
} | ||
} |
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,57 @@ | ||
import type { Client} from '@openfeature/web-sdk'; | ||
import { ProviderEvents } from '@openfeature/web-sdk'; | ||
import type { Client, Provider } from '@openfeature/web-sdk'; | ||
import { NOOP_PROVIDER, ProviderEvents } from '@openfeature/web-sdk'; | ||
import { use } from './use'; | ||
|
||
/** | ||
* A weak map is used to store the global suspense status for each provider. It's | ||
* important for this to be global to avoid rerender loops. Using useRef won't | ||
* work because the value isn't preserved when a promise is thrown in a component, | ||
* which is how suspense operates. | ||
*/ | ||
const globalProviderSuspenseStatus = new WeakMap<Provider, Promise<unknown>>(); | ||
|
||
/** | ||
* Suspends until the client is ready to evaluate feature flags. | ||
* DO NOT EXPORT PUBLICLY | ||
* @param {Client} client OpenFeature client | ||
* | ||
* **DO NOT EXPORT PUBLICLY** | ||
* @internal | ||
* @param {Provider} provider the provider to suspend for | ||
* @param {Client} client the client to check for readiness | ||
*/ | ||
export function suspendUntilReady(client: Client): Promise<void> { | ||
let resolve: (value: unknown) => void; | ||
let reject: () => void; | ||
throw new Promise((_resolve, _reject) => { | ||
resolve = _resolve; | ||
reject = _reject; | ||
client.addHandler(ProviderEvents.Ready, resolve); | ||
client.addHandler(ProviderEvents.Error, reject); | ||
}).finally(() => { | ||
client.removeHandler(ProviderEvents.Ready, resolve); | ||
client.removeHandler(ProviderEvents.Ready, reject); | ||
}); | ||
export function suspendUntilInitialized(provider: Provider, client: Client) { | ||
const statusPromiseRef = globalProviderSuspenseStatus.get(provider); | ||
if (!statusPromiseRef) { | ||
// Noop provider is never ready, so we resolve immediately | ||
const statusPromise = provider !== NOOP_PROVIDER ? isProviderReady(client) : Promise.resolve(); | ||
// Storing the promise globally because | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: This comment is unfinished
beeme1mr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
globalProviderSuspenseStatus.set(provider, statusPromise); | ||
// Use will throw the promise and React will trigger a rerender when it's resolved | ||
use(statusPromise); | ||
} else { | ||
// Reuse the existing promise, use won't rethrow if the promise has settled. | ||
use(statusPromiseRef); | ||
} | ||
} | ||
|
||
/** | ||
* Suspends until the provider has finished reconciling. | ||
* | ||
* **DO NOT EXPORT PUBLICLY** | ||
* @internal | ||
* @param {Client} client the client to check for readiness | ||
*/ | ||
export function suspendUntilReconciled(client: Client) { | ||
use(isProviderReady(client)); | ||
} | ||
|
||
async function isProviderReady(client: Client) { | ||
const controller = new AbortController(); | ||
try { | ||
return await new Promise((resolve, reject) => { | ||
client.addHandler(ProviderEvents.Ready, resolve, { signal: controller.signal }); | ||
client.addHandler(ProviderEvents.Error, reject, { signal: controller.signal }); | ||
}); | ||
} finally { | ||
controller.abort(); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
/// <reference types="react/experimental" /> | ||
// This function is adopted from https://github.com/vercel/swr | ||
import React from 'react'; | ||
|
||
/** | ||
* Extends a Promise-like value to include status tracking. | ||
* The extra properties are used to manage the lifecycle of the Promise, indicating its current state. | ||
* More information can be found in the React RFE for the use hook. | ||
* @see https://github.com/reactjs/rfcs/pull/229 | ||
*/ | ||
export type UsePromise<T> = | ||
Promise<T> & { | ||
status?: 'pending' | 'fulfilled' | 'rejected'; | ||
value?: T; | ||
reason?: unknown; | ||
}; | ||
|
||
/** | ||
* React.use is a React API that lets you read the value of a resource like a Promise or context. | ||
* It was officially added in React 19, so needs to be polyfilled to support older React versions. | ||
* @param {UsePromise} thenable A thenable object that represents a Promise-like value. | ||
* @returns {unknown} The resolved value of the thenable or throws if it's still pending or rejected. | ||
*/ | ||
export const use = | ||
React.use || | ||
// This extra generic is to avoid TypeScript mixing up the generic and JSX syntax | ||
// and emitting an error. | ||
// We assume that this is only for the `use(thenable)` case, not `use(context)`. | ||
// https://github.com/facebook/react/blob/aed00dacfb79d17c53218404c52b1c7aa59c4a89/packages/react-server/src/ReactFizzThenable.js#L45 | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
(<T, _>(thenable: UsePromise<T>): T => { | ||
switch (thenable.status) { | ||
case 'pending': | ||
throw thenable; | ||
case 'fulfilled': | ||
return thenable.value as T; | ||
case 'rejected': | ||
throw thenable.reason; | ||
default: | ||
thenable.status = 'pending'; | ||
thenable.then( | ||
(v) => { | ||
thenable.status = 'fulfilled'; | ||
thenable.value = v; | ||
}, | ||
(e) => { | ||
thenable.status = 'rejected'; | ||
thenable.reason = e; | ||
}, | ||
); | ||
throw thenable; | ||
} | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import React from 'react'; | ||
import { Context } from '../internal'; | ||
import { OpenFeature } from '@openfeature/web-sdk'; | ||
import type { Provider } from '@openfeature/web-sdk'; | ||
import { MissingContextError } from '../internal/errors'; | ||
|
||
/** | ||
* Get the {@link Provider} bound to the domain specified in the OpenFeatureProvider context. | ||
* Note that it isn't recommended to interact with the provider directly, but rather through | ||
* an OpenFeature client. | ||
* @returns {Provider} provider for this scope | ||
*/ | ||
export function useOpenFeatureProvider(): Provider { | ||
const openFeatureContext = React.useContext(Context); | ||
|
||
if (!openFeatureContext) { | ||
throw new MissingContextError('No OpenFeature context available'); | ||
} | ||
|
||
return OpenFeature.getProvider(openFeatureContext.domain); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I am not 100% sure but globals in Next.js code tend to create weird states.
If this code is run on the server
globalProviderSuspenseStatus
will be the same for every caller if I recall correctly.This means when we save a provider here I think we could either get multiple instances of the provider, or the same provider with the same eval context on multiple clients.
But I might be overseeing anything here that avoids this.
This is what I am not sure about if it applies here: vercel/next.js#47605
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.
It's worth noting that Vercel's SWR library also maintains a global state using a weakmap.
https://github.com/vercel/swr/blob/975f99118fc11eedc79accceb6eeaead19753db9/src/_internal/utils/global-state.ts
In our case, the weakmap is useful for handling the noop provider and some minor initialization optimizations. I don't think it would be a big deal if we lose state between requests. Basically, it would behave how it current done.
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.
I think the problem would be more that we might confuse providers.
From the Issue:
E.g. look at TanStack Query: https://tanstack.com/query/latest/docs/framework/react/guides/ssr?from=reactQueryV3#initial-setup
They say:
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.
I imagine that when we have a static context provider in this list, this could become a problem here.
Maybe we have that problem already between different users with our singleton API, which does not have it's roots in this PR 🤔
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.
Looking at
swr
, I see that theWeakMap
maps a cache object to a specific cache state.For me it is not entirely clear if the
provider
stored in our case is unique for the user or even the request.I think
OpenFeature.setProvider
could be a problem in theReact SDK
when used with Next.js.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.
You may be right and perhaps I should update the PR title to not include Next.JS. This PR definitely addresses a build-time issue that's easily reproducible with Next. However, as you stated, we'll likely need to do more to properly support Next.
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.
Totally!
The problem is that running Next with the React SDK currently can lead to security problems if I understand correctly.