-
Notifications
You must be signed in to change notification settings - Fork 280
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
Conversation
Oh my- very very cool, but did you not see #599 ? 😬 |
Oh yes, I saw that, the |
Did you use node-compat? The CloudFlare workers team tested that branch with success
|
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. |
Even so, still curious if you tried with node-compat? |
Hey, sorry I missed the notification ! Yes, I did try it to run it with wranger and 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 Later on, I solved Supabase by patching the Main blocking issue I've found was because of data types: digest would wait for a Uint8Array (from Also, Cloudflare complained once I tried to deploy that some 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 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.
|
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. |
51a8eb8
to
ca8fd16
Compare
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 |
I don't know why it still complains. Mind resolving the conflicts, and removing the md5 implementation? |
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 :
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 |
Thanks a lot for testing @superboss224 .. I'll take a look tonight and try to deploy myself |
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. |
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. |
Yes exactly ! They prefer the new More info about the two different flags here : 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 |
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 😬 |
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 ! |
@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? |
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
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.
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}|:)|` + |
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.
Just some linting issues - I've went ahead and used eslint
on this file but no kudos :(
false, | ||
['deriveBits'] | ||
), | ||
keylen * 8, |
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.
Nothing changed here
return type === 'sha256' | ||
? Crypto.subtle.digest('SHA-256', x) | ||
: Crypto.subtle.digest('MD5', x) | ||
if (type !== 'sha256') |
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.
Removed MD5 as you asked
await Crypto.subtle.importKey('raw', key, { name: 'HMAC', hash: 'SHA-256' }, false, ['sign']), | ||
textEncoder.encode(x) | ||
)) | ||
update: (x) => ({ |
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.
Linting only
}) | ||
}) | ||
} | ||
|
||
export const process = { |
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.
added at the end of the file because of hrtime
polyfill
@@ -0,0 +1,22 @@ | |||
// Add your database url and run this file with |
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.
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 |
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.
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) => |
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.
linter
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.
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) => |
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.
linter
const process = x.includes('process.') | ||
? 'import { process } from \'../polyfills.js\'\n' | ||
: '' | ||
const polyfills = [ |
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.
Reworked transpile.cf.js to allow for a single ../polyfills.js import (eslint was not happy about the multiple imports)
Hello,
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 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, |
it's not working for Cloudflare pages when using with sveltekit |
Still having this issue on sveltekit, is there something i've to setup? |
@superboss224 @jahir9991 @JustMrMendez According to your logs @superboss224, I believe you all have the same issues. You may need to specify a
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']
// }
// }
}); |
@JustMrMendez
|
Interestingly enough, it seems that using I have something that works with sveltekit / cloudflare / postgres Option 1 - Request changes on official
|
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 |
@JustMrMendez what did you expect playing with bleeding edge stuff? 😅 Your package manager (pnpm) shouldn't run the @wackfx I haven't published cf support to npm yet so that's why 😉 |
This comment was marked as resolved.
This comment was marked as resolved.
cf support is not released in the npm published version yet so you have to install from |
@wackfx I've asked cloudflare what the recommended usage is, so let's see what they say :) |
It installed, but getting back the |
@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. But in the end, when it works (and if it works?) - don't you feel relieved ? 🙂 |
Nope, just removed postgres packages, am trying to make my setup work without it for the moment being, I'll update soon |
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.. |
not sure if i am missing anything or not ? build failed in prod.... Using @sveltejs/adapter-cloudflare |
@porsager @wackfx However, there is new errors now :
In the vite.config.js of my SvelteKit project, i'm using:
but it still doesn't resolve the two modules with 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 ? |
The answer is in my previous message, you should look at the code I linked.
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. |
@jahir9991 Did you ran the |
@wackfx Is that it? |
Yes. You can have the flag on Cloudflare, if your build system locally doesn't allow 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). |
@wackfx 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, |
ohh nice looks like WinterCG is doing some specification work on @porsager what do you think about publishing a new Postgres.js version with the changes in this PR? |
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:
master
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'cloudflare:socket
to please bundlersnet.Socket()
withasync net.Socket()
to allow for dynamic import on CF codeos
fs
polyfills that doesn't respect the original contract - but since we're on cloudflare there is not much more we can doprocess.hrtime
node:
prefixesHow 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: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.