-
Notifications
You must be signed in to change notification settings - Fork 783
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
Fix pages functions using unenv aliased packages #8021
Fix pages functions using unenv aliased packages #8021
Conversation
🦋 Changeset detectedLatest commit: 7350ca0 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/13324132685/npm-package-wrangler-8021 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/8021/npm-package-wrangler-8021 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13324132685/npm-package-wrangler-8021 dev path/to/script.js Additional artifacts:cloudflare-workers-bindings-extension: wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13324132685/npm-package-cloudflare-workers-bindings-extension-8021 -O ./cloudflare-workers-bindings-extension.0.0.0-v9ca029b6f.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v9ca029b6f.vsix create-cloudflare: npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13324132685/npm-package-create-cloudflare-8021 --no-auto-update @cloudflare/kv-asset-handler: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13324132685/npm-package-cloudflare-kv-asset-handler-8021 miniflare: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13324132685/npm-package-miniflare-8021 @cloudflare/pages-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13324132685/npm-package-cloudflare-pages-shared-8021 @cloudflare/unenv-preset: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13324132685/npm-package-cloudflare-unenv-preset-8021 @cloudflare/vite-plugin: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13324132685/npm-package-cloudflare-vite-plugin-8021 @cloudflare/vitest-pool-workers: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13324132685/npm-package-cloudflare-vitest-pool-workers-8021 @cloudflare/workers-editor-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13324132685/npm-package-cloudflare-workers-editor-shared-8021 @cloudflare/workers-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13324132685/npm-package-cloudflare-workers-shared-8021 @cloudflare/workflows-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13324132685/npm-package-cloudflare-workflows-shared-8021 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts
Outdated
Show resolved
Hide resolved
The good news is that our test coverage seems to check these changes! |
Thanks for the review, I'll try to see why the tests aren't successful when they're executed on the CI, my first guess would be the use of transitive includes (cross-fetch and cross-env)? |
packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts
Outdated
Show resolved
Hide resolved
add missing comparison
handle nullish 'default' and ensure enumerable properties
remove unnecessary packages from fixture
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.
Thanks a lot for your work on this - and coping with my reviews ;)
One last comment and this should be good to go
use toEqual instead of toMatchInlineSnapshot
Use direct 'require' instead of intermediate dependency
@0xD34DC0DE Sorry I was bad at following up here. Thanks for your work here and let's get this in today |
Congratulations @0xD34DC0DE, the maintainer of this repository has issued you a holobyte! Here it is: https://holopin.io/holobyte/cm74jng4052780clav2yk5rnz This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account. |
Merci Émerick and sorry again for letting this unattended for so long :( |
Fixes #7572
nodejs_compat
(v2) together with the import of an npm package getting aliased by unenv would trigger the addition of__cf_cjs
multiple times__cf_cjs
esm to cjs converter function by an inline expression to avoid the use of esbuild banners that might get included in the bundle multiple time.A separate fixture has been created to avoid making all tests in
fixtures\pages-nodejs-v2-compat
fail for an unrelated reason since the runtime fails to start when the issue is present.The test fixture was inspired by the
nodejs-hybrid-app
andpages-nodejs-v2-compat
fixtures.