Skip to content
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

draft: Cloudflare works ! 🎉 #618

Merged
merged 2 commits into from
Jul 5, 2023
Merged

Conversation

wackfx
Copy link
Contributor

@wackfx wackfx commented Jun 23, 2023

Hey there !

Amazing work with your lib. It's so awesome that I was really sad to see that it didn't work on cloudflare 😢

I've spent some time to make it work on Cloudflare. Tested on my own project - it's working for both Cloudflare pages workers and regular workers.

This PR is not ready to merge - but since I've spent hours trying to deploy this library on Cloudflare based on your work - I guess you may want to take some pointers to what's missing from your implementation to successfully deploy it.

Here's what changed:

  • Rebased your current work on master
  • polyfill: Crypto.subtle.digest(MD5) didn't work as md5 is not supported anymore. Added Joseph's Myers implementation to follow your pattern of 'no external libs'
  • polyfill: Added dynamic import of cloudflare:socket to please bundlers
  • transpile: Replacing net.Socket() with async net.Socket() to allow for dynamic import on CF code
  • polyfill: os fs polyfills that doesn't respect the original contract - but since we're on cloudflare there is not much more we can do
  • polyfill: process.hrtime
  • transpile: Buffer being undefined on cloudflare - added import on build:cf
  • transpile: added node: prefixes

How to use ?

Simply import postgres from 'postgres' and instantiate it like you would do in a regular node env.

I use this module with Qwik and the adapter cloudflare-pages.
You'll need to tell bundler (through vite.config.ts on Qwik) to resolve workerd and set the node library (but compatible with cf workers) as external like this:
image

That's it. Feel free to take whatever you want and close this PR, I won't take it personally, I know it's pretty hacky ;)
Thanks again for your initial work.

@porsager
Copy link
Owner

Oh my- very very cool, but did you not see #599 ? 😬

@wackfx
Copy link
Contributor Author

wackfx commented Jun 24, 2023

Oh yes, I saw that, the cloudflare branch was actually the base work for this (your commits are in my branch!), but it didn't work when deployed on Cloudflare 😞

@porsager
Copy link
Owner

porsager commented Jun 24, 2023 via email

@porsager
Copy link
Owner

Ok, back at computer. I should have read your entire description, I just didn't had time when I answered ;) I'll look closer later or tomorrow.

@porsager
Copy link
Owner

Even so, still curious if you tried with node-compat?

@wackfx
Copy link
Contributor Author

wackfx commented Jun 25, 2023

Hey, sorry I missed the notification !

Yes, I did try it to run it with wranger and node_compat, with a simple JS test like this:

import postgres from './cf/src/index'
const DB_URI = "..." // URI
const sql = postgres(DB_URI)
export function onRequest(context) {
  console.log(await sql`SELECT * FROM users`)
  return new Response("Hello, world!")
}

Unfortunately, tried on both Railway and Supabase, it didn't work. It blocked at the authentication.

I did solve Railway by patching the md5 function (because the authentication sent by the server was a standard unprotected one)

Later on, I solved Supabase by patching the sha256 method because Supabase asked for a SASL authentication.

Main blocking issue I've found was because of data types: digest would wait for a Uint8Array (from decode/encode) and I was giving it a string - which made the whole thing freeze. Also, the connection.js logic required to get back an ArrayBuffer (and avoid any other type) to be functional.

Also, Cloudflare complained once I tried to deploy that some globals or modules was missing (because polyfill wouldn't add them). You really have to try to deploy something to make sure it works: sometimes it worked perfectly locally, but then Cloudflare would complain at deploy time.

I've my project rn running on Cloudflare Pages (with some workers functions) - I can give you the link if you want 😄

EDIT: Also my configuration: I'm using Qwik, which uses Vite / Rollup under the hood to pack the worker script, so some modifications I've made are only to please those bundlers, like the dynamic import of cloudflare:sockets in net.Socket() which Vite wouldn't let go unless it was dynamically imported.

EDIT 2: I believe this comment had the same issues as I had. I'm building for Cloudflare pages which looks more restrictive than regular workers.
In order to debug I did this:

  • Link the local package with npm link
  • Go to my Qwik project (which will use pages) and npm link postgres there to have my local code used
  • Build the project with bun run build
  • Test the bundled codewith bunx wrangler pages dev ./dist --compatibility-date=2023-06-20 --log-level=debug --compatibility-flag=nodejs_compat

@porsager
Copy link
Owner

porsager commented Jun 25, 2023

Very cool you're getting these last things in order. I wanted to merge mine quite some time ago, but haven't had the time (luckily).

Mind rebasing your changes on top of my cloudflare branch to get a better view of what changed? (just rebased my cloudflare branch on master now)

Also wrt. md5, I'm not gonna include that part, but instead just not support md5 auth on cf then.

@wackfx
Copy link
Contributor Author

wackfx commented Jun 26, 2023

Hey !

I've rebased on top of your cloudflare branch - sorry about the formatting issues.

Let me know if I can be of any more help 🙂

Thank you

@porsager porsager changed the base branch from cloudflare to master June 26, 2023 07:22
@porsager porsager changed the base branch from master to cloudflare June 26, 2023 07:22
@porsager
Copy link
Owner

I don't know why it still complains. Mind resolving the conflicts, and removing the md5 implementation?

@superboss224
Copy link

Hello, I've tested today's commits on the cloudflare branch, and it's not enough for Cloudflare Pages

I may be enough for Cloudflare Workers, which has the node_compat flag that polyfills some native node modules, but it doesn't work for the new, up to date, nodejs_compat flag that is the only one usable with Cloudflare Pages (for full stack projects)

I'm still getting this error log :

16:33:51.546 | > Using @sveltejs/adapter-auto
16:33:51.852 | ✘ [ERROR] Could not resolve "os"
16:33:51.852 |  
16:33:51.853 | node_modules/postgres/src/index.js:1:15:
16:33:51.853 | 1 │ import os from 'os'
16:33:51.854 | ╵                ~~~~
16:33:51.854 |  
16:33:51.854 | The package "os" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
16:33:51.854 |  
16:33:51.854 | ✘ [ERROR] Could not resolve "fs"
16:33:51.855 |  
16:33:51.855 | node_modules/postgres/src/index.js:2:15:
16:33:51.855 | 2 │ import fs from 'fs'
16:33:51.855 | ╵                ~~~~
16:33:51.855 |  
16:33:51.855 | The package "fs" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
16:33:51.855 |  
16:33:51.856 | ✘ [ERROR] Could not resolve "stream"
16:33:51.858 |  
16:33:51.858 | node_modules/postgres/src/large.js:1:19:
16:33:51.858 | 1 │ import Stream from 'stream'
16:33:51.858 | ╵                    ~~~~~~~~
16:33:51.858 |  
16:33:51.858 | The package "stream" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
16:33:51.859 |  
16:33:51.865 | ✘ [ERROR] Could not resolve "net"
16:33:51.866 |  
16:33:51.866 | node_modules/postgres/src/connection.js:1:16:
16:33:51.866 | 1 │ import net from 'net'
16:33:51.866 | ╵                 ~~~~~
16:33:51.867 |  
16:33:51.867 | The package "net" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
16:33:51.867 |  
16:33:51.867 | ✘ [ERROR] Could not resolve "tls"
16:33:51.867 |  
16:33:51.867 | node_modules/postgres/src/connection.js:2:16:
16:33:51.867 | 2 │ import tls from 'tls'
16:33:51.867 | ╵                 ~~~~~
16:33:51.867 |  
16:33:51.868 | The package "tls" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
16:33:51.868 |  
16:33:51.868 | ✘ [ERROR] Could not resolve "crypto"
16:33:51.868 |  
16:33:51.869 | node_modules/postgres/src/connection.js:3:19:
16:33:51.869 | 3 │ import crypto from 'crypto'
16:33:51.869 | ╵                    ~~~~~~~~
16:33:51.869 |  
16:33:51.869 | The package "crypto" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
16:33:51.869 |  
16:33:51.869 | ✘ [ERROR] Could not resolve "stream"
16:33:51.869 |  
16:33:51.870 | node_modules/postgres/src/connection.js:4:19:
16:33:51.870 | 4 │ import Stream from 'stream'
16:33:51.870 | ╵                    ~~~~~~~~
16:33:51.870 |  
16:33:51.871 | The package "stream" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
16:33:51.871

So, we can't consider that Postgres.js is yet truly working with cloudflare

I think that this new, more restrictive nodejs_compat flag requires the module to have no nodejs native module imported (like os, fs, etc..) and that it will need further polyfilling/modifications to make it work

To test if it works, simply deploy a project that uses Postgres.js with Functions on pages, and use the nodejs_compat flag

Please let me know if you get a different result than mine on Cloudflare Pages, I'm really looking forward to using Postgres.js on it

@porsager
Copy link
Owner

Thanks a lot for testing @superboss224 .. I'll take a look tonight and try to deploy myself

@wackfx
Copy link
Contributor Author

wackfx commented Jun 26, 2023

I believe @superboss224 had the same issues that I've fixed (looking at the import errors he gave). Definitely a disprecancy between Cloudflare Worker's and Page's workers.
Although depending on the database he uses, he may need some tinkering with the code.

@wackfx
Copy link
Contributor Author

wackfx commented Jun 26, 2023

Sorry I'm not on my computer right now to fix the issues. May be able to take a look tomorrow. I've got this code running actually on Cloudflare Pages with Qwik (https://shwink.wackfx.com) so I do believe it's working.

@superboss224
Copy link

I believe @superboss224 had the same issues that I've fixed (looking at the import errors he gave). Definitely a disprecancy between Cloudflare Worker's and Page's workers. Although depending on the database he uses, he may need some tinkering with the code.

Yes exactly !
Cloudflare Workers has a more powerful node_compat = true flag that also includes polyfills for native module, but that flag is getting more or less legacy, and I don't think that Cloudflare wants to continue supporting it actively in the future.

They prefer the new compatibility_flags = [ "nodejs_compat" ]

More info about the two different flags here :
https://developers.cloudflare.com/workers/wrangler/configuration/#add-polyfills-using-wrangler

The difference in the doc may seem subtle, but it is what is causing the discrepancy between Worker and Pages

(I'm not sure that the new nodejs_compat is necessary if everything can be polyfilled, but that new flag should work in Pages

@porsager
Copy link
Owner

Things are moving fast.. Deprecating things within months 😂 Well good thing we didn't even release yet so we can get over on the new thing 😬

@superboss224
Copy link

Yep, giga fast, there's a recent switch to push to that truly "javascript edge runtime" that has nothing to do with node.js or native modules (on Vercel Edge, Cloudflare and other platforms)

But we can now use a real tcp connexion in those runtimes and it's awesome because they are closer to the end user and have almost no cold start (meanwhile Vercel Functions has 2-3 seconds cold start and an user will feel it)

And being able to use a DBs in those edge runtimes was the last piece missing to run entire Full Stacks apps on it and avoid downtimes/scaling issues/latency for good, so having Postgres.js working with it will be awesome !

@porsager
Copy link
Owner

porsager commented Jul 2, 2023

@wackfx been looking at this, and was quite hard to diff with the formatting changes. I've updated my branch with the things I could glance was necessary from your changes. Do you see anything missing, or perhaps just want to try it out?

Also, it would be great to have sample projects for both workers and pages to do proper tests. Do you have some minimal setups to test with?

@porsager porsager changed the base branch from cloudflare to master July 2, 2023 21:28
@porsager porsager changed the base branch from master to cloudflare July 2, 2023 21:28
feat: reran transpile
fix linter
feat: final touches + test files

squashed 2 commits
fix: Polyfills bulk (to please linter)
fix: Removed MD5 + put back SHA in the digest()

squashed 5 commits
fix: cloudflare workers deployment
feat: fixed auth
fix: encrypt not found in worker :(
fix: postgres SASL
fix: linting
Copy link
Contributor Author

@wackfx wackfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some PR comments

`(?:${v6Seg}:){1}(?:(:${v6Seg}){0,4}:${v4Str}|(:${v6Seg}){1,6}|:)|` +
`(?::((?::${v6Seg}){0,5}:${v4Str}|(?::${v6Seg}){1,7}|:))` +
')(%[0-9a-zA-Z-.:]{1,})?$'
`(?:${v6Seg}:){7}(?:${v6Seg}|:)|` +
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some linting issues - I've went ahead and used eslint on this file but no kudos :(

false,
['deriveBits']
),
keylen * 8,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing changed here

return type === 'sha256'
? Crypto.subtle.digest('SHA-256', x)
: Crypto.subtle.digest('MD5', x)
if (type !== 'sha256')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed MD5 as you asked

await Crypto.subtle.importKey('raw', key, { name: 'HMAC', hash: 'SHA-256' }, false, ['sign']),
textEncoder.encode(x)
))
update: (x) => ({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linting only

})
})
}

export const process = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added at the end of the file because of hrtime polyfill

@@ -0,0 +1,22 @@
// Add your database url and run this file with
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file gives some instructions to test the code on pages wrangler (notice the pages in the command)

@@ -0,0 +1,15 @@
// Add your database url and run this file with
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regular wrangler test

@@ -1,38 +1,47 @@
import fs from 'fs'
import path from 'path'

const empty = x => fs.readdirSync(x).forEach(f => fs.unlinkSync(path.join(x, f)))
, ensureEmpty = x => !fs.existsSync(x) ? fs.mkdirSync(x) : empty(x)
const empty = (x) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linter

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you're using the .eslint from this repo?

, root = 'cf'
, src = path.join(root, 'src')

ensureEmpty(src)

fs.readdirSync('src').forEach(name =>
fs.readdirSync('src').forEach((name) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linter

const process = x.includes('process.')
? 'import { process } from \'../polyfills.js\'\n'
: ''
const polyfills = [
Copy link
Contributor Author

@wackfx wackfx Jul 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked transpile.cf.js to allow for a single ../polyfills.js import (eslint was not happy about the multiple imports)

@superboss224
Copy link

Hello,
I use SvelteKit and looking again at the logs :

21:15:48.711 | > Using @sveltejs/adapter-auto
-- | --
21:15:49.005 | ✘ [ERROR] Could not resolve "os"
21:15:49.005 |  
21:15:49.005 | node_modules/postgres/src/index.js:1:15:
21:15:49.005 | 1 │ import os from 'os'

It seems like that it doesn't want to import from /cf/src folder, but rather directly from /src, so it can't use the polyfilled code at all, so it crashes

I tried to update my vite.config.js with export conditions :

import { sveltekit } from '@sveltejs/kit/vite';

/** @type {import('vite').UserConfig} */
const config = {
  resolve: {
    conditions: ['workerd', 'webworker', 'worker', 'browser', 'import', 'module', 'default']
  },
  plugins: [sveltekit()],
  server: {
    port: 3000
  }
}

export default config;

However, it is still doing the same thing when building on cloudflare, and not importing from /cf/src

Maybe it's a bug from SvelteKit's building process, because even in local, I didn't find any way of using /cf/src

Is there something I could do better about that ? or another way to "force import" from /cf/src without having to fork the package to a version that only have /cf/src ? I tried import postgres from 'postgres/cf/src/index.js' but it gives an error

Maybe there is a way to tell cloudflare or vite to choose /cf/src ?

It would be awesome if I could get some help to use the cloudflare version with SvelteKit,
Best regards

@jahir9991
Copy link

jahir9991 commented Aug 23, 2023

it's not working for Cloudflare pages when using with sveltekit
any update on that plz?

@JustMrMendez
Copy link

Still having this issue on sveltekit, is there something i've to setup?

@wackfx
Copy link
Contributor Author

wackfx commented Aug 23, 2023

@superboss224 @jahir9991 @JustMrMendez

According to your logs @superboss224, I believe you all have the same issues.
You need to use the workerd entrypoint, not the 'regular' node_modules/postgres/src/index.js 

You may need to specify a resolve key in the vite configuration, like this:

resolve: {
      conditions: ['workerd', 'webworker', 'worker', 'browser', 'import', 'module', 'default'],
},

Please refer to Vite / Sveltekit documentation, this is part of my code (not using SvelteKit) that actually works on Cloudflare Pages.

@JustMrMendez
Copy link

@superboss224 @jahir9991 @JustMrMendez

According to your logs @superboss224, I believe you all have the same issues. You need to use the workerd entrypoint, not the 'regular' node_modules/postgres/src/index.js 

You may need to specify a resolve key in the vite configuration, like this:

resolve: {
      conditions: ['workerd', 'webworker', 'worker', 'browser', 'import', 'module', 'default'],
},

Please refer to Vite / Sveltekit documentation, this is part of my code (not using SvelteKit) that actually works on Cloudflare Pages.

tried that too, did not worked:

export default defineConfig({
	plugins: [sveltekit()],
	resolve: {
		conditions: ['workerd', 'webworker', 'worker', 'browser', 'import', 'module', 'default']
	}
	// build: {
	// 	rollupOptions: {
	// 		external: ['cloudflare:sockets', 'cloudflare:events', 'node:stream', 'node:buffer']
	// 	}
	// }
});

@wackfx
Copy link
Contributor Author

wackfx commented Aug 23, 2023

@JustMrMendez
i can see rollupOptions in your code, try with this:

    build: {
      rollupOptions: {
        external: [
          'cloudflare:sockets',
          'node:events', // not cloudflare
          'node:stream',
          'node:buffer'
        ]
   }

@JustMrMendez
Copy link

JustMrMendez commented Aug 23, 2023

external: [
'cloudflare:sockets',
'node:events', // not cloudflare
'node:stream',
'node:buffer'
]

I also have the compatibility flags, wrangler.toml etc...
image
Same error, this are my logs:

> Using @sveltejs/adapter-cloudflare
--
14:10:03.207 | ✘ [ERROR] Could not resolve "os"
14:10:03.207 |  
14:10:03.208 | node_modules/.pnpm/postgres@3.3.5/node_modules/postgres/src/index.js:1:15:
14:10:03.208 | 1 │ import os from 'os'
14:10:03.210 |~~~~
14:10:03.211 |  
14:10:03.211 | The package "os" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
14:10:03.211 |  
14:10:03.214 | ✘ [ERROR] Could not resolve "fs"
14:10:03.215 |  
14:10:03.215 | node_modules/.pnpm/postgres@3.3.5/node_modules/postgres/src/index.js:2:15:
14:10:03.215 | 2 │ import fs from 'fs'
14:10:03.216 | ╵                ~~~~
14:10:03.216 |  
14:10:03.216 | The package "fs" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
14:10:03.216 |  
14:10:03.218 | ✘ [ERROR] Could not resolve "stream"
14:10:03.218 |  
14:10:03.218 | node_modules/.pnpm/postgres@3.3.5/node_modules/postgres/src/large.js:1:19:
14:10:03.218 | 1 │ import Stream from 'stream'
14:10:03.218 |~~~~~~~~
14:10:03.218 |  
14:10:03.219 | The package "stream" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
14:10:03.219 |  
14:10:03.235 | ✘ [ERROR] Could not resolve "net"
14:10:03.236 |  
14:10:03.236 | node_modules/.pnpm/postgres@3.3.5/node_modules/postgres/src/connection.js:1:16:
14:10:03.236 | 1 │ import net from 'net'
14:10:03.236 | ╵                 ~~~~~
14:10:03.236 |  
14:10:03.237 | The package "net" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
14:10:03.237 |  
14:10:03.245 | ✘ [ERROR] Could not resolve "tls"
14:10:03.246 |  
14:10:03.246 | node_modules/.pnpm/postgres@3.3.5/node_modules/postgres/src/connection.js:2:16:
14:10:03.246 | 2 │ import tls from 'tls'
14:10:03.246 |~~~~~
14:10:03.246 |  
14:10:03.247 | The package "tls" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
14:10:03.247 |  
14:10:03.247 | ✘ [ERROR] Could not resolve "crypto"
14:10:03.247 |  
14:10:03.247 | node_modules/.pnpm/postgres@3.3.5/node_modules/postgres/src/connection.js:3:19:
14:10:03.247 | 3 │ import crypto from 'crypto'
14:10:03.247 | ╵                    ~~~~~~~~
14:10:03.247 |  
14:10:03.248 | The package "crypto" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
14:10:03.248 |  
14:10:03.248 | ✘ [ERROR] Could not resolve "stream"
14:10:03.248 |  
14:10:03.248 | node_modules/.pnpm/postgres@3.3.5/node_modules/postgres/src/connection.js:4:19:
14:10:03.248 | 4 │ import Stream from 'stream'
14:10:03.248 |~~~~~~~~
14:10:03.249 |  
14:10:03.249 | The package "stream" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
14:10:03.249 |  
14:10:03.286 | error during build:
14:10:03.286 | Error: Build failed with 7 errors:
14:10:03.287 | node_modules/.pnpm/postgres@3.3.5/node_modules/postgres/src/connection.js:1:16: ERROR: Could not resolve "net"
14:10:03.287 | node_modules/.pnpm/postgres@3.3.5/node_modules/postgres/src/connection.js:2:16: ERROR: Could not resolve "tls"
14:10:03.287 | node_modules/.pnpm/postgres@3.3.5/node_modules/postgres/src/connection.js:3:19: ERROR: Could not resolve "crypto"
14:10:03.287 | node_modules/.pnpm/postgres@3.3.5/node_modules/postgres/src/connection.js:4:19: ERROR: Could not resolve "stream"
14:10:03.288 | node_modules/.pnpm/postgres@3.3.5/node_modules/postgres/src/index.js:1:15: ERROR: Could not resolve "os"
14:10:03.288 | ...
14:10:03.288 | at failureErrorWithLog (/opt/buildhome/repo/node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:1649:15)
14:10:03.288 | at /opt/buildhome/repo/node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:1058:25
14:10:03.288 | at /opt/buildhome/repo/node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:1003:52
14:10:03.288 | at buildResponseToResult (/opt/buildhome/repo/node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:1056:7)
14:10:03.289 | at /opt/buildhome/repo/node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:1085:16
14:10:03.289 | at responseCallbacks.<computed> (/opt/buildhome/repo/node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:703:9)
14:10:03.289 | at handleIncomingPacket (/opt/buildhome/repo/node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:762:9)
14:10:03.289 | at Socket.readFromStdout (/opt/buildhome/repo/node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:679:7)
14:10:03.289 | at Socket.emit (node:events:514:28)
14:10:03.289 | at addChunk (node:internal/streams/readable:343:12)
14:10:03.319 | ELIFECYCLE  Command failed with exit code 1.
14:10:03.359 | Failed: Error while executing user command. Exited with error code: 1
14:10:03.370 | Failed: build command exited with code: 1
14:10:09.844 | Failed: error occurred while running build command

@wackfx
Copy link
Contributor Author

wackfx commented Aug 23, 2023

Interestingly enough, it seems that using npm install postgres doesn't actually install the version containing the cloudflare compatible code, ./cf folder is nowhere to be found (cc @porsager).

I have something that works with sveltekit / cloudflare / postgres

image

Option 1 - Request changes on official adapter-cloudflare plugin

The official plugin is missing some really important options to make this work.

  • Can't override external. This is important you may want to use node modules available on Pages with the node-compatibility flag. It's missing the node flags we require
  • Can't override conditions. Same thing, it doesn't cost that much to let advanced user the import conditions. As it is, it's missing the workerd condition that this project uses.

They basically already have an option object that is really under used. I don't think those changes would be hard for them to accept (those are just regular build options).

for @porsager: It looks like the official module uses worker instead of workerd entrypoint. Not sure what's the standard and which project could change here, but wanted to make you notice this difference that might cause user issues.

Option 2 - Use the custom adapter I made

Basically took the official adapter - and adapted it to be compatible with node / worker (source code).

  • Run npm install https://github.com/porsager/postgres to install Postgres from GitHub (and have the cf folder)
  • Run npm install -D https://github.com/wackfx/adapter-cloudflare-node to install my custom adapter
  • Change svelte.config.js to this
// change this line
// import adapter from '@sveltejs/adapter-cloudflare';

// with this line (only add -node at the end)
import adapter from '@sveltejs/adapter-cloudflare-node';

// It should work with the default kit configuration
/** @type {import('@sveltejs/kit').Config} */
const config = {
	kit: {
		adapter: adapter(),
	}
};

Please, if you have any issue with this custom adapter, post it on my repository. Not here

@JustMrMendez
Copy link

man, what a nightmare this has been:

 ERR_PNPM_PREPARE_PACKAGE  Failed to prepare git-hosted package fetched from
 "https://codeload.github.com/porsager/postgres/tar.gz/838c8daa89568e60161d6cee7d14c2ac26b696f1":
 postgres@3.3.5 prepublishOnly: `npm run lint`
spawn ENOENT

@porsager
Copy link
Owner

@JustMrMendez what did you expect playing with bleeding edge stuff? 😅 Your package manager (pnpm) shouldn't run the prepublishOnly script on install. It couldn't be more clearly named, but for some reason they might have thought that would be necessary when installing from github 🫠 I've moved on from pnpm myself because of annoyances like these, but if I recall correctly you can add --ignore-scripts to the install command to avoid any scripts running.

@wackfx I haven't published cf support to npm yet so that's why 😉

@jahir9991

This comment was marked as resolved.

@porsager
Copy link
Owner

you may be didn't build the master as there is no cf folder when we install postgres from npmjs

cf support is not released in the npm published version yet so you have to install from porsager/postgres to try it out.

@porsager
Copy link
Owner

It looks like the official module uses worker instead of workerd entrypoint. Not sure what's the standard and which project could change here, but wanted to make you notice this difference that might cause user issues.

@wackfx I've asked cloudflare what the recommended usage is, so let's see what they say :)

@JustMrMendez
Copy link

@JustMrMendez what did you expect playing with bleeding edge stuff? 😅 Your package manager (pnpm) shouldn't run the prepublishOnly script on install. It couldn't be more clearly named, but for some reason they might have thought that would be necessary when installing from github 🫠 I've moved on from pnpm myself because of annoyances like these, but if I recall correctly you can add --ignore-scripts to the install command to avoid any scripts running.

@wackfx I haven't published cf support to npm yet so that's why 😉

It installed, but getting back the ERROR: Could not resolve "*", and yes, this is what you get for living in the edge, i was just frustrated, because i was getting errors/problems on non-related things, like pnpm

@wackfx
Copy link
Contributor Author

wackfx commented Aug 24, 2023

@JustMrMendez we're all about edges here, like our beloved Cloudflare friends 💛

Is your problem fixed now at least ? Is it working? You forgot to tell us about this.
Sure I can understand the frustration, believe me when I was debugging this thing yesterday, it was a pain (and I don't even use SvelteKit).

But in the end, when it works (and if it works?) - don't you feel relieved ? 🙂

@JustMrMendez
Copy link

@JustMrMendez we're all about edges here, like our beloved Cloudflare friends 💛

Is your problem fixed now at least ? Is it working? You forgot to tell us about this.
Sure I can understand the frustration, believe me when I was debugging this thing yesterday, it was a pain (and I don't even use SvelteKit).

But in the end, when it works (and if it works?) - don't you feel relief ? 🙂

Nope, just removed postgres packages, am trying to make my setup work without it for the moment being, I'll update soon

@porsager
Copy link
Owner

Ok, I got word from Cloudflare it should be exports.worker instead of exports.workerd, so I''ve changed that now.. Let me know if that solves the issues..

@jahir9991
Copy link

not sure if i am missing anything or not ?
using "@sveltejs/adapter-cloudflare-node": "https://github.com/wackfx/adapter-cloudflare-node",
"postgres": "https://github.com/porsager/postgres",

build failed in prod....
should i need to add anything in vite.config ?

Using @sveltejs/adapter-cloudflare
19:52:17.809 | ✘ [ERROR] Could not resolve "postgres"
19:52:17.809 |  
19:52:17.809 | .svelte-kit/output/server/chunks/hooks.server.js:4:21:
19:52:17.809 | 4 │ import postgres from "postgres";
19:52:17.810 | ╵ ~~~~~~~~~~
19:52:17.810 |  
19:52:17.810 | The module "./cf/src/index.js" was not found on the file system:
19:52:17.810 |  
19:52:17.810 | node_modules/postgres/package.json:9:14:
19:52:17.810 | 9 │ "worker": "./cf/src/index.js",
19:52:17.811 | ╵ ~~~~~~~~~~~~~~~~~~~
19:52:17.811 |  
19:52:17.811 | You can mark the path "postgres" as external to exclude it from the bundle, which will remove this error.
19:52:17.811


@superboss224
Copy link

@porsager @wackfx
Hello, I've just tested with exports.worker and it now solves the issue, specifying resolve conditions isn't even necessary anymore, it works and now loads from /cf

However, there is new errors now :

18:00:34.662 | ✘ [ERROR] Could not resolve "node:events"
18:00:34.662 |  
18:00:34.663 | node_modules/postgres/cf/polyfills.js:1:29:
18:00:34.663 | 1 │ import { EventEmitter } from 'node:events'
18:00:34.663 | ╵                              ~~~~~~~~~~~~~
18:00:34.663 |  
18:00:34.663 | The package "node:events" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.
18:00:34.663 |  
18:00:34.663 | ✘ [ERROR] Could not resolve "node:buffer"
18:00:34.664 |  
18:00:34.664 | node_modules/postgres/cf/polyfills.js:2:23:
18:00:34.664 | 2 │ import { Buffer } from 'node:buffer'
18:00:34.664 | ╵                        ~~~~~~~~~~~~~
18:00:34.664

In the vite.config.js of my SvelteKit project, i'm using:

  build: {
		rollupOptions: {
			external: ['cloudflare:sockets', 'cloudflare:events', 'node:stream', 'node:buffer']
		}
	},

but it still doesn't resolve the two modules with node:

Maybe there's a cloudflare flag to use on the project to load those two modules ? or something else you can do to solve the issue on your side ?

@wackfx
Copy link
Contributor Author

wackfx commented Aug 25, 2023

@superboss224

The answer is in my previous message, you should look at the code I linked.

Can't override external. This is important you may want to use node modules available on Pages with the node-compatibility flag. It's missing the node flags we require

esbuild is called by the cloudflare adapter, which doesn't allow for any options. You're only left with either asking them to add the option, or use a custom adapter like I made.

@wackfx
Copy link
Contributor Author

wackfx commented Aug 25, 2023

@jahir9991
I don't know how you installed the packages, but I believe it's an installation issue, my package should still work as it doesn't override the worker usage.

Did you ran the npm install I linked ?

@superboss224
Copy link

@wackfx
Okay so, the flag exists, and node:buffer / node:stream should work on Cloudflare Pages but esbuild in the cloudflare adapter just don't let those node: modules work properly, so it nullifies the flag's effect, and I should ask Cloudflare to take their flag into account into their own adapter to fix the bug

Is that it?

@wackfx
Copy link
Contributor Author

wackfx commented Aug 25, 2023

@superboss224

esbuild in the cloudflare adapter just don't let those node: modules work properly, so it nullifies the flag's effect, and I should ask Cloudflare to take their flag into account into their own adapter to fix the bug

Yes. You can have the flag on Cloudflare, if your build system locally doesn't allow node: modules because esbuild is configured to target non-node environments, it won't work.

EDIT: About your message, Cloudflare is not the issue here. The package @sveltekit/cloudflare-page is. You should ask the maintainer of the package to solve this (or temporarily use my package I linked previously to make this issue go away).

@superboss224
Copy link

@wackfx
Okay so, it looks like it's already going to get fixed :
sveltejs/kit#10544

And he also already has his temporary driver to "pre-fix" that

So it should work seamlessly (if there's no other errors) when the pull request is done, all we have to do is wait a little more,
thank you for your help

@karlhorky
Copy link
Contributor

ohh nice looks like WinterCG is doing some specification work on connect(), maybe it becomes a cross-runtime standard 😮

@porsager what do you think about publishing a new Postgres.js version with the changes in this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants