-
Notifications
You must be signed in to change notification settings - Fork 62
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: migrate to cloudflare pages and vite (Work in progress) #81
Conversation
Wow, awesome – I took a stab at this for a month+ ago and didn't end up finishing. Let me know how I can help! |
app/analytics/collect.test.ts
Outdated
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.
Trying to understand on how to update this tests
Hi @benvinegar, Whenever you get sometime, could you please checkout this branch and test? I don't have data for my cloudflare account, hence not able to test the dashboard completely. Apart from |
@BhanuNexus I think I addressed the tests. But I don't know how to actually run this. If I try It looks like this is converted to Cloudflare Pages (e.g. the
Maybe I'm missing some steps/instructions? |
@benvinegar could you please try below? # added prepreview script now
npm run preview |
Thank you :) |
favicon.ico to reduce localhost 404
Yep, this gets the server running -- thank you -- but looks like it's producing a bad query because it can't extract the timezone correctly. Poking at this. |
Ok, I think it works: the only remaining issue is figuring out how to inject This doesn't seem to ever get a value:
Any ideas? |
Here's your PR branch running locally, serving my production dataset: counterscale-cf-pages.mov |
Trying to deploy this and I basically just get 500 Internal Error with no visibility into what's going on / no actual HTML returned or anything. |
Thank you very much for your changes, sorry for trouble with wrong parameter for timezone. I am assuming there are no secrets set, can you try below? # for production
npx wrangler pages secret put CF_ACCOUNT_ID --env production
npx wrangler pages secret put CF_BEARER_TOKEN --env production
# for preview
npx wrangler pages secret put CF_ACCOUNT_ID --env preview
npx wrangler pages secret put CF_BEARER_TOKEN --env preview I'm able to run it here https://counterscaleapp.pages.dev |
…r local deployment
This value is available with CF build environment (similar to github actions variables) |
Sorry will try to poke at this tonight. I do wish there was better reporting coming out of pages or a way to capture this without blowing up. |
Yep, that did it 🎉
^Note I didn't need this I think because (I think) it reads Okay, so my thinking is: this is basically a major version. It's basically a re-architecture of what I'm used to, and I'm not super comfortable just making this So here's my suggestion:
@BhanuNexus Thoughts on this plan? |
yeah, this is an issue and this also makes me think of using Cloudflare Pages settings instead of considering |
Totally agreed. |
Just following up – I've been on vacation, but back now and plan to make these branch changes soon.
👍 |
@@ -130,7 +130,7 @@ interface DataPoint { | |||
// More here: https://developers.cloudflare.com/analytics/analytics-engine/get-started/#limits | |||
|
|||
export function writeDataPoint( | |||
analyticsEngine: CFAnalyticsEngine, | |||
analyticsEngine: AnalyticsEngineDataset, |
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 generally speaking, this change isn't really related to migrating to vite
, and more of a separate personal choice that's been inserted (same with renaming Environment
to Env
).
I'll keep it (it's more clear I suppose) but generally it's best to not to couple these changes.
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.
Agreed, this AnalyticsEngineDataset
type is directly from @cloudflare/workers-types
So, syntaxes and change tracking are aligned with CF.
Regarding Env
, I personally prefer Environment
whatever you have set but cloudflare auto generates types from wrangler.toml
to worker-configuration.d.ts
by using wrangler types
, that is the reason I have removed Environment
and reused Env
from worker-configuration.d.ts
"dev": "remix vite:dev", | ||
"build": "remix vite:build", | ||
"prepreview": "remix vite:build", | ||
"preview": "wrangler pages dev ./build/client", |
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.
When should I use npm run preview
and when should I use npm run dev
? Are the remix
commands deprecated now?
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.
wrangler pages dev ./build/client
command simulate the CF environment locally like functions
Where as remix vite:build
just works with remix, usually this command alone will have 404 for /collect
endpoint of CF functions.
If we manage to migrate /collect
endpoint to remix, then preview wouldn't be needed anymore. I believe this is doable with minor changes, I will try to get this done in couple of days.
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.
Hi @benvinegar, I have previously added this endpoint to test movement of /collect from CF function to remix.
I have tested this with mine https://cs.usevega.com and seems to be working fine, do you want to have this change included with this PR or may be in later iteration?
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.
@BhanuNexus I started the v2
branch for this ongoing migration (here). I'd make a new PR targeting that.
I'm going to close this PR in the meantime.
Closing because this PR has become the |
@BhanuNexus All your changes are now on |
Migrate to Remix Vite and Cloudflare Pages
Status: Work in progress
Issue: #55