-
Notifications
You must be signed in to change notification settings - Fork 702
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
Update Next.js template to support bindings #4688
Conversation
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7534950857/npm-package-wrangler-4688 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7534950857/npm-package-wrangler-4688 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7534950857/npm-package-wrangler-4688 dev path/to/script.js Additional artifacts:npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7534950857/npm-package-miniflare-4688 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7534950857/npm-package-cloudflare-pages-shared-4688 npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7534950857/npm-package-create-cloudflare-4688 --no-auto-update Note that these links will no longer work once the GitHub Actions artifact expires.
| Please ensure constraints are pinned, and |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4688 +/- ##
==========================================
+ Coverage 75.63% 75.70% +0.06%
==========================================
Files 243 243
Lines 13210 13222 +12
Branches 3393 3396 +3
==========================================
+ Hits 9992 10010 +18
+ Misses 3218 3212 -6 |
@irvinebroque another thing that needs to be updated in c3 for Next.js (which I was planning to do as soon as the "pages:build": "npx @cloudflare/next-on-pages",
"pages:deploy": "npm run pages:build && wrangler pages deploy .vercel/output/static",
- "pages:watch": "npx @cloudflare/next-on-pages --watch",
- "pages:dev": "npx wrangler pages dev .vercel/output/static --compatibility-date=2023-12-18 --compatibility-flag=nodejs_compat"
+ "pages:preview": "npm run pages:build && npx wrangler pages dev .vercel/output/static --compatibility-date=2023-12-18 --compatibility-flag=nodejs_compat" so that then developers can follow the recommended workflow and:
As I said I was planning to do this as soon as next-dev's ready, so I am really happy to do it in a followup PR, or maybe the change could be included here, whatever you prefer 🙂 (in any case I thought you'd want to be aware of this) |
@dario-piotrowicz is there an ETA on when then module might not be experimental? Is it possible to ship while it's experimental? |
Yeah it'd be possible to ship this while the package is experimental, but anyways as ETA for making the module non-experimental I'd say in the next few days, next week at the very very latest, it's just a matter of merging this PR: cloudflare/next-on-pages#612 and making a next-on-pages release 🙂 |
@irvinebroque @admah the next-on-pages |
e9a092d
to
328288b
Compare
// Note: this simple implementation doesn't handle tsconfigs containing comments | ||
// (as it is not needed for the existing use cases) |
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 implemented this quick solution just to get things working with Next.js, before merging I should explore if we can also, without too much complications, handle jsonc tsconfigs
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.
Updating jsonc files doesn't seem very simple, I can see that in wrangler it was already considered not worth it:
workers-sdk/packages/wrangler/src/init.ts
Lines 462 to 465 in ccf14dd
// We don't update the tsconfig.json because | |
// it could be complicated in existing projects | |
// and we don't want to break them. Instead, we simply | |
// tell the user that they need to update their tsconfig.json |
Here as well a generic solution might end up too complex and not worth it...
I did try to experiment a bit with json5 here but mixed success (the tsconfig can get parsed by during the parsing the comments are lost)
I think would keep this as is for now (since currently it is good enough for Next.js) and improve things later if need be 🤔
9f6289c
to
a72da1c
Compare
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 pulled this PR down and used it to create a Next.js application locally.
I uncommented the various KV binding bits and added to the pages:preview
script.
Then npm run dev
and npm run pages:preview
all work as expected.
I can also see that the typings are working as expected!
Nice!
What
Updates starter template that running
npm create cloudflare
and selecting Next.js creates, to make use of bindings proxy, as implemented innext-on-pages
.Why
Provides a starting point that shows how to work with bindings in local dev, even when local dev server is in Node.js.
Before merging
next.config.js
file be written? https://github.com/cloudflare/workers-sdk/pull/4688/files#r1439845985@cloudflare/workers-types
is installed as a dev dependency for apps using TSdev
for local developmentbuild
for building the applicationpages:preview
for previewing the pages app locally (basically the currentpages:dev
)pages:deploy
for deploying the pages apppages:build
for building the pages application (run as a pre-set bypages:preview
andpages:deploy
)pages:dev
(since users should just usedev
now)tsconfig.json
files (see comment)refs irvinebroque/next-bindings-test#1
refs #4523