cloudflare / next-on-pages

CLI to build and develop Next.js apps for Cloudflare Pages
https://www.npmjs.com/package/@cloudflare/next-on-pages
MIT License
1.29k stars 126 forks source link

message: Reflect.get called on non-object on Drizzle ORM #499

Closed divyam234 closed 7 months ago

divyam234 commented 1 year ago

next-on-pages environment related information

message: Reflect.get called on non-object

Description

message: Reflect.get called on non-object

Calling any drizzle methods which need table schema is throwing this error although it is working on vercel.

const  users = db.table('users', {
  id: serial('id').primaryKey(),
  name: text('name'),
});
await db.select().from(users);
await db.execute(sql`select * from users;`)

This statement throws Reflect Error but calling raw query through db.execute() is working fine.

I am using kysely sql builder in one of my project with @vercel/postgres its working fine here but only happens with drizzle.

dario-piotrowicz commented 1 year ago

Hi @divyam234 thanks for the issue, sorry for the late reply

I've had a look and tried using DrizzleORM with next-on-pages with a turso db and from a quick look things seem to be working fine: Screenshot 2023-10-27 at 11 23 26

Screenshot 2023-10-27 at 11 20 42

So I don't believe this to be a generic issue with Drizzle ORM, I think it can likely something triggered by your specific configuration.

Could you please clarify how to reproduce the issue? If you could provide a minimal reproduction (with instructions on how to set up the db and trigger the issue) that would be extremely helpful and would allow me to go and investigate this properly 🙏

divyam234 commented 1 year ago

@dario-piotrowicz sry for late response. As I mentioned above in issue its happening with postgres driver mainly @vercel/postgres , sql drivers are working fine. Here is a reproduction https://github.com/divyam234/next-pages-issue .Its working fine on next dev but not on pages dev . Just create a vecel postgres or neon db to test reproduction

james-elicx commented 1 year ago

The problem seems to be that it is trying to access process.env.POSTGRES_URL outside of the AsyncLocalStorage for some reason.

image

The proxy object constructed here is actually the sql variable that is imported from @vercel/postgres. This specific part can be fixed by using the createPool() function instead of the sql variable in drizzle(createPool(), { schema }).

Then there appears to be a second part that causes issues, but this time inside the @neondatabase/serverless library.

image

But I think this part that throws an error is actually from the pg library that Neon imports in their library.

So, the potential solution could be modifying our process.env proxy trap to return {} when the envAsyncLocalStorage.getStore() returns undefined, but that isn't fixing the issue, that's just a flimsy bandaid, because you won't be able to access environment variables in places that result in this error since they don't seem to have access to the AsyncLocalStorage context for some reason.

What do you think about this @dario-piotrowicz?

IIRC, it's not really possible to get Postgres to work properly in Pages in the first place.

dario-piotrowicz commented 1 year ago

@james-elicx I haven't tried but I think that postgres should work fine in Workers (as indicated here in the docs), so I'd assume that it also works in Pages, or is there something specific that makes it not work with Pages?

If pg is having issues there I would assume that it'd need to be fixed upstream, but since it is related to env it really feels likely that it could be related to/caused by the first issue, I would try to solve that first and deal with pg afterwards if it still throws.


Regarding the ALS issue, all the code we run is running inside the ALS context so I think that probably there isn't anything actually wrong in next-on-pages, it looks to me like we're loosing the Async Context because of a runtime bug, we've already had a similar issue in the past so it wouldn't be too surprising to me.

We could add the try-catch around getStore() back (and returning undefined as you suggested), but as you said it's going to be a bandaid at best, but if it helps the situation, even barely it might be worth it to temporarily add it in? 🤷 (PS: I wouldn't want to add it permanently in as I think that seeing this sort of breakage is actually valuable)

In terms of a proper solution I think we might need to contact the runtime team and see if they can look into it, the problem is that we can't just give them a big/complex-ish next-on-pages + dependencies application since it would make it much more difficult for them to debug it and find the issue. (Also before bothering them we should actually be pretty sure that it is a runtime bug and not a next-on-pages specific one, I think that most likely it is, but we should make sure). So before contacting them we will need to try to create a Worker (or Pages) minimal reproduction of this (that involves as little extra dependencies and complexities as possible).


@james-elicx let me know if you'd like to have a try on creating a minimal (ideally Worker, Pages otherwise) reproduction or if I should give it a go 🙂

Also, did you notice any particular flow here that could potentially generate the issue? (like the process.env being used in a particular context, etc...)

james-elicx commented 1 year ago

@james-elicx I haven't tried but I think that postgres should work fine in Workers (as indicated here in the docs), so I'd assume that it also works in Pages, or is there something specific that makes it not work with Pages?

all i see in discord when it comes to postgres and pages is people saying something isn't working properly /shrug

Regarding the ALS issue, all the code we run is running inside the ALS context so I think that probably there isn't anything actually wrong in next-on-pages, it looks to me like we're loosing the Async Context because of a runtime bug, we've already had a similar issue in the past so it wouldn't be too surprising to me.

We could add the try-catch around getStore() back (and returning undefined as you suggested), but as you said it's going to be a bandaid at best, but if it helps the situation, even barely it might be worth it to temporarily add it in? 🤷 (PS: I wouldn't want to add it permanently in as I think that seeing this sort of breakage is actually valuable)

getStore() is returning undefined in these situations where it loses the async local storage value, so try-catch wouldn't change anything, unless you wanted to put try-catch around Reflect.get(...).

In terms of a proper solution I think we might need to contact the runtime team and see if they can look into it, the problem is that we can't just give them a big/complex-ish next-on-pages + dependencies application since it would make it much more difficult for them to debug it and find the issue. (Also before bothering them we should actually be pretty sure that it is a runtime bug and not a next-on-pages specific one, I think that most likely it is, but we should make sure). So before contacting them we will need to try to create a Worker (or Pages) minimal reproduction of this (that involves as little extra dependencies and complexities as possible).

I mean, I don't see why it would be a next-on-pages bug. We are lazy loading the function's file while running inside the ALS, and other usage in the function's file keep the context. I would have thought that it isn't possible to break out of being run inside the async local storage when it's done like this, but then again, I don't know how the internals work. I hadn't thought of it being a runtime issue, but that would make sense, just not sure how we can strip this back to make it an even more minimal repro!

@james-elicx let me know if you'd like to have a try on creating a minimal (ideally Worker, Pages otherwise) reproduction or if I should give it a go 🙂

Also, did you notice any particular flow here that could potentially generate the issue? (like the process.env being used in a particular context, etc...)

Trying to access Object.keys(process.env) inside the proxy getter in the sql export from https://github.com/vercel/storage/blob/main/packages/postgres/src/index.ts would throw the error. Not sure how easy that will be to reproduce though because it seems like something that should work fine, but wasn't with how it was used in the reproduction provided.

sassanh commented 11 months ago

I'm getting the same error about Reflect.get with openai library, I thought maybe this screenshot can help you understand what's going on without needing a minimal reproduction repository.

image image
sassanh commented 11 months ago

I added this script entry in my package.json:

    "cloudflare_build": "npx @cloudflare/next-on-pages@1 && sed -i 's/process.env.DEBUG/true/g' .vercel/output/static/_worker.js/__next-on-pages-dist__/functions/api/ai/build-query.func.js"

and changed my build command in Cloudflare to this:

npm run cloudflare_build

as a workaround until this issue is fixed.

dario-piotrowicz commented 11 months ago

Thanks so much @sassanh 🙏

Unfortunately your screenshots show more or less the same I've seen when debugging the original issue 😓, but this is still valuable as it shows that the issue is not specific to Drizzle and also looking into the openai library could surface some similarities that can help clarify what's going wrong here 🤔

Is there any chance you could provide a minimal repro of your issue with the openai library? or if that's not possible some instructions on how I can reproduce the error? 🙏

sassanh commented 11 months ago

I prepared a repo, I hope it helps.

Reproduction steps:

  1. Clone this repository and cd into its directory:

    git clone git@github.com:sassanh/cloudflare-nextjs-issue.git
    cd cloudflare-nextjs-issue
  2. Install dependencies and build it:

    npm install
    npx @cloudflare/next-on-pages@1
  3. Run wrangler, note than you should put real API key here:

    npx wrangler pages dev .vercel/output/static --compatibility-flag=nodejs_compat --binding OPENAI_API_KEY=<REAL_API_KEY_HERE>
  4. Call the endpoint:

    curl http://localhost:8788/api/some-endpoint -d '{}'
dario-piotrowicz commented 11 months ago

Awesome! 😄 thanks so much @sassanh 🙏 I will have a look soon 🙂

maghfoorx commented 11 months ago

Hey hey guys, I posted my issue on discord but was asked to share it here as well :D Pretty much Im getting the Reflect.get error as well when Im making a call to openai when my application is deployed on cloudflare using next-on-pages.

When I run my built app using wrangler I can see the error and also see where it is produced.

It's in packages/next-on-pages/src/buildApplication/generateGlobalJs.ts on line 25

// .wrangler/tmp/pages-OgYkpx/bundledWorker-0.624464237701096.mjs
import("node:buffer")
  .then(({ Buffer: Buffer2 }) => {
    globalThis.Buffer = Buffer2;
  })
  .catch(() => null);
var __ENV_ALS_PROMISE__ = import("node:async_hooks")
  .then(({ AsyncLocalStorage }) => {
    globalThis.AsyncLocalStorage = AsyncLocalStorage;
    const envAsyncLocalStorage = new AsyncLocalStorage();
    globalThis.process = {
      env: new Proxy(
        {},
        {
          ownKeys: () => Reflect.ownKeys(envAsyncLocalStorage.getStore()),
          getOwnPropertyDescriptor: (_2, ...args) =>
            Reflect.getOwnPropertyDescriptor(
              envAsyncLocalStorage.getStore(),
              ...args
            ),
          get: (_2, property) =>
            Reflect.get(envAsyncLocalStorage.getStore(), property),
          set: (_2, property, value) =>
            Reflect.set(envAsyncLocalStorage.getStore(), property, value),
        }
      ),
    };
    return envAsyncLocalStorage;
  })
  .catch(() => null);

I've realised my issue is the same as @sassanh. apologies for the repeat 😅

maghfoorx commented 11 months ago

I added this script entry in my package.json:

    "cloudflare_build": "npx @cloudflare/next-on-pages@1 && sed -i 's/process.env.DEBUG/true/g' .vercel/output/static/_worker.js/__next-on-pages-dist__/functions/api/ai/build-query.func.js"

and changed my build command in Cloudflare to this:

npm run cloudflare_build

as a workaround until this issue is fixed.

Can confirm this works!!

janpio commented 11 months ago

Same problem when using @neondatabase/serverless via Prisma.

Using the same code outside of @cloudflare/next-on-pages context works both for Cloudflare Workers and Cloudflare Pages (we have continuous tests for that at https://github.com/prisma/ecosystem-tests/tree/dev/driver-adapters-wasm/neon-cf-basic and https://github.com/prisma/ecosystem-tests/tree/dev/driver-adapters-wasm/neon-cfpages-basic).

Here is the full error I am getting:

✘ [ERROR] TypeError: Reflect.get called on non-object

      at [object Object]
      at Object.get (file:///workspace/next-pg/.vercel/output/static/_worker.js/index.js:19:37)
      at val
  (file:///workspace/next-pg/.vercel/output/static/_worker.js/__next-on-pages-dist__/functions/api/neon.func.js:2:4587)
      at new ConnectionParameters
  (file:///workspace/next-pg/.vercel/output/static/_worker.js/__next-on-pages-dist__/functions/api/neon.func.js:2:5176)
      at new Client
  (file:///workspace/next-pg/.vercel/output/static/_worker.js/__next-on-pages-dist__/functions/api/neon.func.js:2:35556)
      at new NeonClient
  (file:///workspace/next-pg/.vercel/output/static/_worker.js/__next-on-pages-dist__/functions/api/neon.func.js:2:63520)
      at gi.newClient
  (file:///workspace/next-pg/.vercel/output/static/_worker.js/__next-on-pages-dist__/functions/api/neon.func.js:2:48495)
      at gi.connect
  (file:///workspace/next-pg/.vercel/output/static/_worker.js/__next-on-pages-dist__/functions/api/neon.func.js:2:48445)
      at gi.query
  (file:///workspace/next-pg/.vercel/output/static/_worker.js/__next-on-pages-dist__/functions/api/neon.func.js:2:50902)
      at gi.query
  (file:///workspace/next-pg/.vercel/output/static/_worker.js/__next-on-pages-dist__/functions/api/neon.func.js:2:67858)
  {
    clientVersion: '5.8.0-dev.32'
  }

And the full reproduction repo: https://github.com/janpio/cloudflare-next-prisma/tree/neon Just npm run pages:build && npm run pages:dev, then open /api/neon.

By the way, as it has not been mentioned before: @vercel/postgres uses @neondatabase/serverless under the hood.

Can we somehow help to debug this further?

(Update weirdly this does not reproduce when using @neondatabase/serverless directly without the "wrapper" that Prisma uses: https://github.com/janpio/cloudflare-next-prisma/compare/neon...plain-neon?expand=1 Prisma isn't doing anything weird though really - but somehow it leads to some "context" getting lost or changed in a way this project does not like.)

dario-piotrowicz commented 11 months ago

Thanks a lot @janpio 🙂

I've already shared the original reproduction with the runtime team but they haven't gotten around to looking into it.

Can we somehow help to debug this further?

The only thing that would really help fix this would be to produce a minimal Workers/Pages reproduction which does not use next-on-pages or any external library, if we had that it would be much much easier to ask for the runtime team's assistance into solving this bug (assuming that it is a runtime bug, which I believe it is)

I did try to create such a minimal reproduction (by basically trying to reproduce the whole logic into a bare Cloudflare Worker) but didn't have much luck, I will try my hand at that again this or next week, if you'd also like to try you're more than welcome 🙏

dario-piotrowicz commented 10 months ago

@sassanh your openAI reproduction really helped me a ton making a minimal reproduction to share with the runtime team! Thanks a lot for it! 😄 ❤️

https://github.com/cloudflare/workerd/issues/1513

I'll ping the runtime team and see if this could get fixed soon 🙂, fixing that should definitely fix the openAI reproduction and with a bit of luck the other cases as well 😄 (but I can't really guarantee that right now)

paramaggarwal commented 9 months ago

I am running into this issue when running Prisma (proxied via Accelerate) on Cloudflare Pages:

(error) [auth][cause]: PrismaClientKnownRequestError: 
Invalid `prisma.account.findUnique()` invocation:

Cannot fetch data from service:
Reflect.get called on non-object
    at sl.handleRequestError (__next-on-pages-dist__/webpack/ebf5a7de6e7fba3d1c088441ad870e13.js:34:7228)
    at sl.handleAndLogRequestError (__next-on-pages-dist__/webpack/ebf5a7de6e7fba3d1c088441ad870e13.js:34:6319)
    at sl.request (__next-on-pages-dist__/webpack/ebf5a7de6e7fba3d1c088441ad870e13.js:34:6032)
    at async d (__next-on-pages-dist__/webpack/ebf5a7de6e7fba3d1c088441ad870e13.js:42:5566)
    at async getUserByAccount (__next-on-pages-dist__/webpack/0993d15eb342ee8a60dcc6068a8c1e0e.js:363:47083)
    at async _.<computed> (__next-on-pages-dist__/webpack/0993d15eb342ee8a60dcc6068a8c1e0e.js:1:49034)
    at async ui (__next-on-pages-dist__/webpack/0993d15eb342ee8a60dcc6068a8c1e0e.js:363:28255)
    at async ia (__next-on-pages-dist__/webpack/0993d15eb342ee8a60dcc6068a8c1e0e.js:363:36750)
    at async De (__next-on-pages-dist__/webpack/0993d15eb342ee8a60dcc6068a8c1e0e.js:363:40224)
    at async __next-on-pages-dist__/webpack/35d3a5e175f73130cbe95b1625d116d5.js:10:27876
  (error) [auth][details]: {}
paramaggarwal commented 9 months ago

Hi @dario-piotrowicz we have a workaround mentioned https://github.com/cloudflare/workerd/issues/1513#issuecomment-1925898430, would this now need to be incorporated in the next-on-pages codebase?

dario-piotrowicz commented 9 months ago

Hi @paramaggarwal, the comment there is basically suggesting to update the thenables to use AsyncLocalStorage.snapshot, that's not really something that can be feasibly done in next-on-pages as it would require us to analyse and modify all possible user code (including its dependencies) which I think would be very complex, very very slow and brittle.

The suggested workaround is, in my opinion, pretty impractical as thenables (like in your case) come from libraries and not user code (so users can't really easily change them).

If we were to add a temporary fix for this I suspect that adding the patch to the runtime as suggested in https://github.com/cloudflare/workerd/issues/1513#issuecomment-1874590621 would be a much less painful and costly solution (and would fix the issue not only for next-on-pages users but for all workerd users).


One thing you could try would be to use something like patch-package or pnpm's native patch (or whatever else appropriate for your package manager) to patch prisma so to add the workaround to its thenables (but I am not sure how much effort that could take 😓)

paramaggarwal commented 9 months ago

Thanks for explaining this @dario-piotrowicz - I guess I'll wait for now.

dario-piotrowicz commented 9 months ago

@paramaggarwal thanks for the understanding 🙏

I've left a comment in the workerd issue as well regarding this 🙂

maitreya00 commented 9 months ago

I'm having the same issue using drizzle orm.

itsmereal commented 9 months ago

I'm having the same issue using drizzle orm and turso db.

jellohouse commented 9 months ago

I'm getting this same error TypeError: Reflect.get called on non-object when doing a simple read from my supabase db.

const { data, error } = await supabase
    .from('my-table')
    .select()
    .eq("foo", "bar")

Additionally i see when i do npm run pages:dev and I call this endpoint on my API an error saying "[ERROR] Your worker created multiple branches of a single stream (for instance, by calling response.clone() or request.clone()) but did not read the body of both branches."

dario-piotrowicz commented 9 months ago

Hi @maitreya00 , @itsmereal and @jellohouse 👋 the issue actually lies in the workerd Runtime and not next-on-pages specifically, the runtime team seems to be hopefully going to fix it soon (https://github.com/cloudflare/workerd/issues/1513#issuecomment-1936275341) so please keep an eye on the issue there, once that's solved things should hopefully just work here, if not I'll look into why

dario-piotrowicz commented 9 months ago

@jellohouse the issue regarding response.clone() is unrelated, that's a runtime error (/warning) that happens when a request/response is cloned multiple times without reading their body, basically wasting resources. That I'd imagine has to be an issue either in your code or in the code of one of your dependencies I suspect that there's not too much we can do about it, but if you want please feel free to open a new issue for it and we can discuss further there 👍

dario-piotrowicz commented 8 months ago

@divyam234 I've just tried your reproduction with the latest version of wrangler and it seems to be working as expected 🥳 🎉 🥳

@divyam234, @sassanh, @maghfoor-dev, @janpio and @paramaggarwal please have a look and let me know if it's all good now for you 🙏 🙂

janpio commented 8 months ago

I can confirm via the reproduction that I had shared above that it goes from [ERROR] TypeError: Reflect.get called on non-object to some follow up error of not having an environment variable (which is correct in my reproduction) when I upgrade to wrangler@3.35.0 🥳 Thank you from @prisma!

dario-piotrowicz commented 7 months ago

@janpio since this issue seems to have been resolved, do you think Neon is going to work with next-on-pages? 🙂

If so maybe we could update the docs here? https://www.prisma.io/docs/orm/prisma-client/deployment/edge/deploy-to-cloudflare#deploying-a-nextjs-app-with-cloudflarenext-on-pages

what do you think? 🙂

dario-piotrowicz commented 7 months ago

I'm closing this issue as the bug should be completely fixed now 🙂

(anyone who might disagree please comment here or feel free to open a new issue)