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

add next-dev internal-package #486

Merged
merged 21 commits into from
Nov 3, 2023

Conversation

dario-piotrowicz
Copy link
Member

@dario-piotrowicz dario-piotrowicz commented Oct 5, 2023

Idea for supporting bindings mocking in next dev using miniflare's new magic proxy API.

Usage

(more details are in the new module's README)

In the next.config.js just add the following:

 /** @type {import('next').NextConfig} */
 const nextConfig = {}

+ const { setupDevBindings } = require('@cloudflare/next-on-pages/next-dev');

+ setupDevBindings({
+     kvNamespaces: [...],
+     durableObjects: {MY_DO: {className: 'MyDo', scriptName: ''}},
+     // ...r2 and d1
+ });

 module.exports = nextConfig

and then running npm run dev (a.k.a. next dev) will populate the process.env with the bindings defined in the config file

Notes

  • No runtime impact, no extra code at all makes it into the bundled worker
  • next dev gets the binding mocks but it's still important to note that it is still not very representative of production, so it's still advised to check things with wrangler pages dev before deploying
  • all the next dev goodness is present, including fast reloading and HMR 🚀

Small very minimal demos/examples that use this new implementation:


resolves #271

@changeset-bot
Copy link

changeset-bot bot commented Oct 5, 2023

⚠️ No Changeset found

Latest commit: f4d6fc7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dario-piotrowicz dario-piotrowicz marked this pull request as draft October 5, 2023 17:47
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

🧪 Prereleases are available for testing 🧪

@cloudflare/next-on-pages

You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/6739263039/npm-package-next-on-pages-486

@cloudflare/eslint-plugin-next-on-pages

You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/6739263039/npm-package-eslint-plugin-next-on-pages-486

@dario-piotrowicz dario-piotrowicz force-pushed the dev-bindings branch 3 times, most recently from cfdbbc5 to 3ca3028 Compare October 6, 2023 13:08
@dario-piotrowicz dario-piotrowicz changed the title add dev-bindings internal-package add next-dev internal-package Oct 12, 2023
@millar
Copy link

millar commented Oct 14, 2023

Hello @dario-piotrowicz,

Thank you for your work on this, which is going to make my development workflow 100x better. I have been testing this out in one of my projects to mock D1 and I have noticed that the mock database file has a different hash to that of the main wrangler commands.

This causes issues as when I run my migrations via wrangler d1 migrations apply db_name --local they do not apply to the same database as is used via this mock (I have one at e7352547963de7050bd7d94658afc4fe78b61811b7815da12d90be8e863abf4d.sqlite and one at d4f889bae404efba2a3527fb440041c852e355c21bf33365fdd1939d27442be8.sqlite in the same .wrangler state folder). Do you know why this could be and if there is a workaround to have wrangler use the same database file as the one your mock uses?

Cheers!

@dario-piotrowicz
Copy link
Member Author

Hi @millar, first of all thanks so very much for trying this out! I hope it's been working nicely for you! 😄

Regarding the D1 db, my mocks use miniflare under the hood for them which is what also wrangler uses, so the two should have the same exact results.... so I don't really know what you're experiencing that issue, maybe this depends on some options set for miniflare or something like that... I'll have to have a look

Anyways just to be sure, did you check what happens when you run your application with wrangler pages dev? (if you didn't could you please? 🙏) is the d1 db recognized in that case?

@mrbbot
Copy link
Contributor

mrbbot commented Oct 19, 2023

@millar One thing you'll need to be careful of is that the database UUID passed to setupDevBindings() matches the UUID used by wrangler. The UUID should be the preview_database_id if specified, otherwise the database_id.


For the following wrangler.toml...

[[d1_databases]]
binding = "DB"
database_name = "database"
database_id = "3eab3f70-03c0-41d2-81f2-9fc363117677"

...you'll want something like...

setupDevBindings({
  d1Databases: {
    DB: "3eab3f70-03c0-41d2-81f2-9fc363117677"
  },
});

...whereas for the following wrangler.toml with a preview_database_id...

[[d1_databases]]
binding = "DB"
database_name = "database"
database_id = "3eab3f70-03c0-41d2-81f2-9fc363117677"
preview_database_id = "00b88e8f-9cf3-4b92-8dfa-76d98a26c3b2"

...you'll want to use that instead...

setupDevBindings({
  d1Databases: {
    DB: "00b88e8f-9cf3-4b92-8dfa-76d98a26c3b2"
  },
});

@dario-piotrowicz dario-piotrowicz marked this pull request as ready for review October 23, 2023 16:46
const { kvNamespaces, r2Buckets, d1Databases, services, textBindings } =
options;
const bindings = {
bindings: textBindings,
Copy link
Member Author

Choose a reason for hiding this comment

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

@IgorMinar, I'm totally happy to rename our textBindings argument to bindings if we don't want to introduce a new term

That's also how they are referred to in wrangler pages dev:
Screenshot 2023-10-23 at 17 50 58

I find the terminology pretty confusing but I understand introducing a new one not being great, so I'm totally ok with renaming this to bindings if you prefer 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

PS: Indeed I don't think we use textBinding or anything alike anywhere else...

@iamvishnusankar
Copy link

iamvishnusankar commented Oct 24, 2023

@dario-piotrowicz Thanks a lot for this. Local version works fine but having issues with deploying to Cloudflare pages. I'm using bun and it's throwing install errors.

Plus, queue binding will be a great addition to this.

Screenshot 2023-10-25 at 2 06 16 AM

@dario-piotrowicz
Copy link
Member Author

@dario-piotrowicz Thanks a lot for this. Local version works fine but having issues with deploying to Cloudflare pages. I'm using bun and it's throwing install errors.

I'm not sure what the issue is... does bun support installs from non-registry sources such as our prerelease links? (if it works locally I guess it does? maybe only in some OSes/versions or something? 😕:thinking:)

I'll try to look into it but I guess it could be a bug in bun itself? in the meantime, when you have to use prereleases maybe you could switch to npx <cloudflare_prerelease> (just in the Pages integration)?

Plus, queue binding will be a great addition to this.

Thanks for the callout! I'll look into adding queues as well 🙂👍

@james-elicx
Copy link
Contributor

@dario-piotrowicz Thanks a lot for this. Local version works fine but having issues with deploying to Cloudflare pages. I'm using bun and it's throwing install errors.

Bun has had various issues with our prerelease registry links for many months sadly.

bun x / bunx straight up just doesn't work - oven-sh/bun#3675

@iamvishnusankar
Copy link

As @james-elicx mentioned, it looks like this issue is with bun itself.

What's weird is that it works perfectly fine on local machine but fails while deploying to pages. As per the suggestion by @dario-piotrowicz I've modified the pre-build script for pages deployment and it's now using npm for deploying to Cloudflare pages. Here's what I've done

  1. Set environment variables (For both preview & production)
SKIP_DEPENDENCY_INSTALL=true
UNSTABLE_PRE_BUILD=rm -rf bun.lockb && npm install
  1. Update build script to use npm
npx @cloudflare/next-on-pages@1

While this works for the deployment, it strips away the package installation consistency offered by lock files. So, the deployed app is prone to bugs introduced by patches or minor versions of dependencies. And next.js sometimes breaks things even in the patches.

A better solution is to deploy the pre-release versions to npm registry so that such annoying tooling issues won't occur. Additionally, I'm having trouble finding the pre-release versions of next-on-pages. It being deployed to npm would allow devs to browse through versions and look for changes without installing the pre version.

@dario-piotrowicz
Copy link
Member Author

Prereleases are something we added to help testing changes, they were never added with the intention of being used for any non-minimal period of time. I don't think publish them to npm would make too much sense, please keep in mind that each every commit on an opened PR "generates" a new prerelease, so if we were to publish them in no time we'd have thousand of useless prerelease versions there (each formatting, comment change, etc... on an open PR would generate one).

What you mentioned as pre version is I think more or less what we call beta, as soon as a PR is merged we indeed publish it to npm so it is usable from there (again, if pre would point to the latest prerelease, what would happen when there are multiple PRs opened at the same time? with each new commit pushed each would become the predominant pre? wouldn't that be extremely confusing?)

Also please note that usually we do merge PRs rather quickly so the need to rely on a prerelease is quite rare I believe (and they are used as I mentioned above just to check things), this one has been a bit different as it is a significant addition and I didn't want to rush it in (potentially getting the API wrong). Sorry about that.

@dario-piotrowicz
Copy link
Member Author

PS: additional personal point of view 😅, this also does feel like a bug in bun to me, if that's the case then instead of us changing thing to accommodate bun's shortcomings the fix should happen in bun itself (unless they clearly state that this is not a bug and that they do intend only to support the standard npm registry and nothing else 🤷)

@james-elicx
Copy link
Contributor

What's weird is that it works perfectly fine on local machine but fails while deploying to pages. As per the suggestion by dario-piotrowicz I've modified the pre-build script for pages deployment and it's now using npm for deploying to Cloudflare pages. Here's what I've done

  1. Set environment variables (For both preview & production)
SKIP_DEPENDENCY_INSTALL=true
UNSTABLE_PRE_BUILD=rm -rf bun.lockb && npm install

@iamvishnusankar you could just leave your package.json as normal with next-on-pages pointing to the latest version. There's no need to use two different ways of installing dependencies for local vs CI... Dario's suggestion was to just use npx https://...... for your build command in your Pages project settings, not change everything to npm install in CI :P

If you want to use the same build command between the two, you could use a package.json script that runs the npx command and use that instead for your build command.

Before we merged our Bun PR, I would have my build command on Pages to use npx https://.... while the Pages CI used Bun for everything.

@iamvishnusankar
Copy link

iamvishnusankar commented Oct 25, 2023

@james-elicx It seems like you misunderstood this situation here.!

The point of using this pre-release version is to get access to setupDevBindings. Which is needed for local development. And as per the examples provided by @dario-piotrowicz , the local package.json needs to be pointed to pre-release version.

It works fine locally, but when being deployed to Cloudflare pages, the CI will auto detect the bun.lock file and will try to auto install dependencies including the pre-release version. And it results in the error I shared in first comment.

To prevent this auto install, I use SKIP_DEPENDENCY_INSTALL and UNSTABLE_PRE_BUILD steps. And I'm using the latest stable version of @cloudflare/next-on-pages@1 as build command as the pre-release has no relevance in building the project.

@james-elicx
Copy link
Contributor

@james-elicx It seems like you misunderstood this situation here.!

The point of using this pre-release version is to get access to setupDevBindings. Which is needed for local development. And as per the examples provided by @dario-piotrowicz , the local package.json needs to be pointed to pre-release version.

It works fine locally, but when being deployed to Cloudflare pages, the CI will auto detect the bun.lock file and will try to auto install dependencies including the pre-release version. And it results in the error I shared in first comment.

To prevent this auto install, I use SKIP_DEPENDENCY_INSTALL and UNSTABLE_PRE_BUILD steps. And I'm using the latest stable version of @cloudflare/next-on-pages@1 as build command as the pre-release has no relevance in building the project.

Oh, derp, you're right, I did misunderstand what you were referring to - totally slipped my mind that you can't use the import if it's not the one in your package.json. My apologies.

@iamvishnusankar
Copy link

iamvishnusankar commented Oct 25, 2023

What you mentioned as pre version is I think more or less what we call beta, as soon as a PR is merged we indeed publish it to npm so it is usable from there (again, if pre would point to the latest prerelease, what would happen when there are multiple PRs opened at the same time? with each new commit pushed each would become the predominant pre? wouldn't that be extremely confusing?)

@dario-piotrowicz No need to push every PR to npm. May be you could setup an action to pre-release the packages when you create a GitHub pre-release with pre-* or experimental-* tag. This will help to selectively publish the pre-release version to npm and devs like me won't have to wait for the PR to get merged to test things out.

@dario-piotrowicz
Copy link
Member Author

@iamvishnusankar ok totally fair point 🙂

Adding an ad-hoc github action to take the latest prerelease from a PR and publish it shouldn't really be too hard, but again I'd be really interested to first see if bun has any intention of fixing this on their side 🤔

@dario-piotrowicz
Copy link
Member Author

@iamvishnusankar I've checked with the team regarding the queue bindings, we agreed that we can just merge this PR without them and add them as a followup (since it's a simple additive change anyways) 🙂

@iamvishnusankar
Copy link

@dario-piotrowicz Awesome, Looking forward for the merge! 🎉

@iamvishnusankar
Copy link

@dario-piotrowicz Hi, any update on this? or a possible experimental/pre version release?

@dario-piotrowicz
Copy link
Member Author

@iamvishnusankar so sorry for this taking a while, the changes are pretty much ready, I just need a review from the team, I'll ping them right now sorry about that! I will try to get this merged by the end of the week! 🙇 🙏

Copy link
Collaborator

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

This looks great! Let's suck it and see.

internal-packages/next-dev/package.json Show resolved Hide resolved
internal-packages/next-dev/src/index.ts Show resolved Hide resolved
internal-packages/next-dev/src/index.ts Outdated Show resolved Hide resolved
internal-packages/next-dev/src/index.ts Outdated Show resolved Hide resolved
internal-packages/next-dev/src/services.ts Outdated Show resolved Hide resolved
dario-piotrowicz and others added 2 commits November 2, 2023 22:13
Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>
@dario-piotrowicz dario-piotrowicz merged commit 4572873 into cloudflare:main Nov 3, 2023
8 checks passed
@dario-piotrowicz dario-piotrowicz deleted the dev-bindings branch November 3, 2023 12:13
@dario-piotrowicz
Copy link
Member Author

@iamvishnusankar PR merged 😄, now you can use this with the latest beta release 🙂

But by the way, so you know, I think the API of how you pass the bindings might still change before we do a proper release (so you might need to slightly tweak that later on)

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.

[⚡ Feature]: Mocking process.env.<BINDING> when running next dev
6 participants