feat: second-cut of using unenv to create a hybrid node.js compatibility setting (via inject) #5878

IgorMinar · 2024-05-21T04:04:45Z

rebased version of https://github.com/cloudflare/workers-sdk/pull/5220/files which uses unenv's inject to introduce globals

Author has addressed the following

Tests
- TODO (before merge)
- Included - the update to the fixture tests this
- Not necessary because:
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required / Maybe required
- Not required because: no e2e tests are affected by node-compat
Changeset (Changeset guidelines)
- TODO (before merge)
- Included
- Not necessary because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Not necessary because: still experimental but we should follow up with a docs ticket.

changeset-bot · 2024-05-21T04:04:49Z

🦋 Changeset detected

Latest commit: 8639e8b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
wrangler	Minor
@cloudflare/vitest-pool-workers	Patch
wrangler-dev-api-app	Patch

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

gitguardian · 2024-05-21T04:04:49Z

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
9963966	Triggered	PostgreSQL Credentials	`61cbf9d`	fixtures/nodejs-hybrid-app/wrangler.toml	View secret
9963966	Triggered	PostgreSQL Credentials	`61cbf9d`	fixtures/nodejs-hybrid-app/worker-configuration.d.ts	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

IgorMinar · 2024-05-21T04:05:40Z

@petebacondarwin I rebased your PR but didn't want to force-push, so I'm creating a new PR. Let's chat tomorrow to discuss how we want to handle these PRs.

packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts

github-actions · 2024-05-21T04:07:04Z

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/9403675692/npm-package-wrangler-5878

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5878/npm-package-wrangler-5878

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9403675692/npm-package-wrangler-5878 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9403675692/npm-package-create-cloudflare-5878 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9403675692/npm-package-cloudflare-kv-asset-handler-5878

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9403675692/npm-package-miniflare-5878

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9403675692/npm-package-cloudflare-pages-shared-5878

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9403675692/npm-package-cloudflare-vitest-pool-workers-5878

Note that these links will no longer work once the GitHub Actions artifact expires.

wrangler@3.59.0 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240524.2
`workerd`	1.20240605.0	1.20240605.0
`workerd --version`	1.20240605.0	2024-06-05

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts

Build #9169531905 from cloudflare/workers-sdk#5878

petebacondarwin · 2024-06-05T08:57:22Z

package.json

@@ -78,7 +78,8 @@
 			"ink@3.2.0": "patches/ink@3.2.0.patch",
 			"toucan-js@3.2.2": "patches/toucan-js@3.2.2.patch",
 			"@cloudflare/component-listbox@1.10.6": "patches/@cloudflare__component-listbox@1.10.6.patch",
-			"capnp-ts@0.7.0": "patches/capnp-ts@0.7.0.patch"
+			"capnp-ts@0.7.0": "patches/capnp-ts@0.7.0.patch",
+			"pg@8.11.3": "patches/pg@8.11.3.patch"


TODO: remove this once it is fixed upstream

petebacondarwin · 2024-06-05T08:58:02Z

packages/wrangler/package.json

@@ -82,6 +82,7 @@
 		"resolve.exports": "^2.0.2",
 		"selfsigned": "^2.0.1",
 		"source-map": "0.6.1",
+		"unenv": "npm:unenv-nightly@1.10.0-1717522572.87b9352",


TODO: update when this is released

yeah. but this won't happen until shortly before we drop the experimental: prefix.

packages/wrangler/src/deploy/deploy.ts

packages/wrangler/src/deployment-bundle/bundle.ts

packages/wrangler/src/deploy/deploy.ts

packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts

petebacondarwin

Last small thing:

a changeset (with a minor bump) - or are we claiming that since this is internal/experimental it should not appear in the changelog? I don't feel we should try to hide it by doing that: users can see the code being added so it is not a secret, and also no changeset means that the package will not be published.

RamIdeas

LGTM just needs the changeset before merge and Pete's comment above addressed

IgorMinar

LGTM except for one regression that was introduced during the refactor. I didn't have enough time to debug it, but I discovered it in the matrix app and moved a reduction into the fixture as 0c2fc6d

somehow don't tell esbuild that node:assert is a built-in, so it freaks out when it's not resolved to an absolute path. did we somehow forget to configure externals?

fixtures/nodejs-hybrid-app/tests/index.test.ts

…ty setting

… - use unenv instead

…pat_v2' accompanied by 'experimental'

…ng in the hybrid plugin

The previous test would pass/fail depending upon which timezone you are in when you run it.

currently this fails with: nodejs-hybrid-app:build: ✘ [ERROR] Plugin "unenv-cloudflare" returned a non-absolute path: node:assert (set a namespace if this is not a file path) nodejs-hybrid-app:build: nodejs-hybrid-app:build: src/index.ts:2:19: nodejs-hybrid-app:build: 2 │ import assert from "node:assert/strict"; nodejs-hybrid-app:build: ╵ ~~~~~~~~~~~~~~~~~~~~ nodejs-hybrid-app:build:

This was problematic because if you import from `node:assert/strict` there is no entry in the `external` map for that path, instead you need to map it to its alias first (e.g. `node:assert`).

… since it mutates compat flags

packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts

packages/wrangler/src/deployment-bundle/node-compat.ts

packages/wrangler/src/dev.tsx

packages/wrangler/src/pages/functions/buildPlugin.ts

RamIdeas

You managed to add this extra compat flag and make the codebase cleaner 👏

petebacondarwin · 2024-06-06T15:47:32Z

I'm going to override the GitGuardian check failure because it is picking up a DB secret that is actually a publicly accessible DB - so a false positive.

fixtures/nodejs-hybrid-app/src/index.ts

fixtures/nodejs-hybrid-app/wrangler.toml

packages/wrangler/package.json

packages/wrangler/src/deployment-bundle/node-compat.ts

penalosa · 2024-06-06T16:06:28Z

packages/wrangler/src/deployment-bundle/node-compat.ts

+		compatibilityFlags.includes("nodejs_compat_v2");
+
+	if (nodejsCompatV2) {
+		// strip the "experimental:" prefix because workerd doesn't understand it yet.


Given the flag is no longer experimental in workerd, do we want to keep this prefix stripping?

No we still want to require users of Wrangler to add this - it was orthogonal to workerd's experimental constraints - and is left here to avoid people trying to use this for production while we stabilize it.

packages/wrangler/src/deployment-bundle/node-compat.ts

IgorMinar requested a review from a team as a code owner May 21, 2024 04:04

IgorMinar requested a review from petebacondarwin May 21, 2024 04:05

IgorMinar commented May 21, 2024

View reviewed changes

packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts Outdated Show resolved Hide resolved

IgorMinar commented May 21, 2024

View reviewed changes

packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts Outdated Show resolved Hide resolved

IgorMinar commented May 21, 2024

View reviewed changes

packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts Outdated Show resolved Hide resolved

petebacondarwin mentioned this pull request May 21, 2024

feat: first-cut of using unenv to create a hybrid node.js compatibility setting #5220

Closed

9 tasks

IgorMinar added a commit to flarelabs-net/workers-nodejs-compat-matrix that referenced this pull request May 21, 2024

build(unenv): update wrangler version to a build from the new PR

316501e

Build #9169531905 from cloudflare/workers-sdk#5878

IgorMinar mentioned this pull request Aug 12, 2024

🐛 BUG: wrangler or miniflare turns on --experimental compatibility flag by default cloudflare/workerd#2518

Open

IgorMinar force-pushed the unenv-wrangler-inject branch from 069d84a to ec101f3 Compare June 4, 2024 02:18

IgorMinar requested a review from a team as a code owner June 4, 2024 02:18

IgorMinar force-pushed the unenv-wrangler-inject branch from df3a491 to 3347c14 Compare June 4, 2024 23:53

petebacondarwin self-assigned this Jun 5, 2024

petebacondarwin reviewed Jun 5, 2024

View reviewed changes

RamIdeas approved these changes Jun 5, 2024

View reviewed changes

IgorMinar commented Jun 6, 2024

View reviewed changes

fixtures/nodejs-hybrid-app/tests/index.test.ts Outdated Show resolved Hide resolved

petebacondarwin and others added 10 commits June 6, 2024 15:36

feat: first-cut of using unenv to create a hybrid node.js compatibili…

61cbf9d

…ty setting

fix up pg cloudflare socket detection

617a949

fix up unenv integration, use inject instead of various hacks

6e425c2

make the side-effects of injected globals tree-shakeable

a4f1e21

mark unenv as external so that it doesn't get bundled into wrangler

3618331

don't configure esbuild with a define to rewrite global to globalThis…

2437267

… - use unenv instead

correctly provide the global polyfill if inject is defined as a string

8c640b8

fix lint issues

c45a91b

add turbo.json file

8b62219

update the activation mechanism to be just a runtime flag 'nodejs_com…

b47cf6c

…pat_v2' accompanied by 'experimental'

petebacondarwin and others added 10 commits June 6, 2024 15:39

Add changeset

461e1ae

Update the fixture and add a test

ae685f9

Separate out the require call conversion from the alias import handli…

55f3f14

…ng in the hybrid plugin

Make sure we don't match packages that don't exist

3d84d9d

Fix formatting on the new fixture test

3c854aa

Fix deploy test snapshot for change to warning

564311d

Fix fixture test to be resilient to timezone changes

7a4c8f4

The previous test would pass/fail depending upon which timezone you are in when you run it.

Match externals against the alias rather than the original path

6de7e89

This was problematic because if you import from `node:assert/strict` there is no entry in the `external` map for that path, instead you need to map it to its alias first (e.g. `node:assert`).

Refactor node-compat validation and handling into a helper function

12b2b8e

petebacondarwin force-pushed the unenv-wrangler-inject branch from 03fdb3a to 12b2b8e Compare June 6, 2024 14:39

petebacondarwin requested a review from RamIdeas June 6, 2024 14:45

Remove extra node-compat validation call, which was messing things up…

b8493ef

… since it mutates compat flags