Conversation
🦋 Changeset detectedLatest commit: 9520b95 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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/10490271832/npm-package-wrangler-6511You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6511/npm-package-wrangler-6511Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10490271832/npm-package-wrangler-6511 dev path/to/script.jsAdditional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10490271832/npm-package-create-cloudflare-6511 --no-auto-updatenpm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10490271832/npm-package-cloudflare-kv-asset-handler-6511npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10490271832/npm-package-miniflare-6511npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10490271832/npm-package-cloudflare-pages-shared-6511npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10490271832/npm-package-cloudflare-vitest-pool-workers-6511npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10490271832/npm-package-cloudflare-workers-editor-shared-6511npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10490271832/npm-package-cloudflare-workers-shared-6511Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
edc058c to
8f5123b
Compare
…eCompat()` This makes it possible to call `validateNodeCompat()` mulitple times without breaking anything.
759e8a3 to
6b4ea80
Compare
|
So excited about this one 👏 |
6b4ea80 to
4c05bc9
Compare
4c05bc9 to
e61cef9
Compare
| const minify = props.minify ?? config.minify; | ||
|
|
||
| const compatibilityFlags = | ||
| props.compatibilityFlags ?? config.compatibility_flags; |
There was a problem hiding this comment.
nit: there are quite a few const someField = props.someField ??/|| config.someFiled, maybe replace with
const resolvedConfig = {...props, ...config}; // x1
const {someField} = resolvedConfig; // x15edit: even const resolvedConfig = {...defaultConfig, ...props, ...config}; // x1
There was a problem hiding this comment.
I'd rather be explicit about which flags can be overridden, rather than hiding them in a general override like that.
There was a problem hiding this comment.
Also note that in props the properties tend to be camelCased while in config they can be snake_cased.
packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts
Outdated
Show resolved
Hide resolved
10eb49e to
5bc168b
Compare
IgorMinar
left a comment
There was a problem hiding this comment.
I haven't reviewed every single line of this PR, but I reviewed the critical parts, and overall structure and that all looks good to me.
Thanks for adding all the test as well!
What this PR solves / how to test
Fixes #6288
You can try it out yourself by pulling down this repo: https://github.com/petebacondarwin/nodejs-2-demo
Author has addressed the following