feat: add support for building Next.JS apps that use suspending flags. #1157
Conversation
Signed-off-by: Michael Beemer <[email protected]>
Signed-off-by: Michael Beemer <[email protected]>
| * 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>>(); |
There was a problem hiding this comment.
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.
It's worth noting that Vercel's SWR library also maintains a global state using a weakmap.
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.
I think the problem would be more that we might confuse providers.
From the Issue:
Since mutating server-side global variables does some combination of a) not updating when you expect them to b) resetting arbitrarily c) leaking memory and d) leaking user data,
E.g. look at TanStack Query: https://tanstack.com/query/latest/docs/framework/react/guides/ssr?from=reactQueryV3#initial-setup
They say:
// NEVER DO THIS:
// const queryClient = new QueryClient()
//
// Creating the queryClient at the file root level makes the cache shared
// between all requests and means all data gets passed to all users.
// Besides being bad for performance, this also leaks any sensitive data.
There was a problem hiding this comment.
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.
Looking at swr, I see that the WeakMap 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 the React SDK when used with Next.js.
There was a problem hiding this comment.
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.
This PR definitely addresses a build-time issue that's easily reproducible with Next.
Totally!
You may be right and perhaps I should update the PR title to not include Next.JS
Removing Next from the title would make sense to me.
The problem is that running Next with the React SDK currently can lead to security problems if I understand correctly.
| 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 |
beeme1mr
changed the title
Signed-off-by: Michael Beemer <[email protected]>
This PR
usepolyfillNotes
Previously, the Next.JS build process would timeout when using a suspense flag hook. The reason for this is because the noop provider (which is used during a build) is never ready. That meant that the promise thrown to initiate suspense never resolved. To address this, I've added global state using a weak map that's keyed off a provider. A
useRefwon't help because the value is only retained if the component renders successfully.How to test
I've added some tests and manually tested in the Toggle Shop.
Note
Marking as a draft until #1152 is merged.