feat(wrangler): use unenv bulitin dependency resolution by vicb · Pull Request #7625 · cloudflare/workers-sdk

vicb · 2024-12-23T10:50:03Z

Reapply of #7541

The initial PR was reverted by #7583

The root caused was fixed in #7614 (via unjs/unenv#378)

/cc @pi0

Tests
- TODO (before merge)
- Tests included
- Tests not necessary because: already tested in https://github.com/cloudflare/workers-sdk/pull/7614/files#diff-dc9521e48f13c8af9737ee8c555eb22b138b148285e843ce1b4b9bdb3b2e3dd7R1
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required
- Not required because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Documentation not necessary because: no user facing changes

changeset-bot · 2024-12-23T10:50:08Z

🦋 Changeset detected

Latest commit: 61c3035

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

This PR includes changesets to release 2 packages

Name	Type
wrangler	Patch
@cloudflare/vitest-pool-workers	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

github-actions · 2024-12-23T11:34:39Z

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/12634299818/npm-package-wrangler-7625

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

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

Or you can use npx with this latest build directly:

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

Additional artifacts:

wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12634299818/npm-package-cloudflare-workers-bindings-extension-7625 -O ./cloudflare-workers-bindings-extension.0.0.0-vdb49d7d66.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-vdb49d7d66.vsix

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

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

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

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

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12634299818/npm-package-cloudflare-unenv-preset-7625

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

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12634299818/npm-package-cloudflare-workers-editor-shared-7625

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12634299818/npm-package-cloudflare-workers-shared-7625

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12634299818/npm-package-cloudflare-workflows-shared-7625

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

wrangler@3.99.0 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20241218.0
`workerd`	1.20241230.0	1.20241230.0
`workerd --version`	1.20241230.0	2024-12-30

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

petebacondarwin · 2025-01-06T15:16:56Z

 				"index.js",
 				`
-      import path from 'path';
+      import path from 'node:path';


why this change?

From the original (reverted) #7541

https://github.com/cloudflare/workers-sdk/pull/7541/files#diff-81706a80fd0b3a4bce16ab020267448ff21ad04a71e22802937633c3080dffbfL10072

petebacondarwin · 2025-01-06T15:17:33Z

+"wrangler": patch
+---
+
+feat(wrangler): use unenv bulitin dependency resolution


Some more explanation here would be helpful.
Using unenv's resolution for what? For all dependencies? Just unenv ones? etc.

I'll fix in a follow-up PR to avoid having to re-run the tests

If you only change the md file then most of the tests are skipped.

But perhaps not in a PR that has other changes...

petebacondarwin · 2025-01-06T15:21:25Z

-			path: aliasAbsolute[args.path],
-			external: external.includes(unresolvedAlias),
+			path: alias[args.path],
+			external: external.includes(unresolved),


Shoudn't the external paths already be resolved?
In which case why use unresolved here, rather than path?

TBH I don't fully get what the code is doing here...

I tried to update the code but revert to the original behavior

petebacondarwin

LGTM - the regression will not re-occur thanks to test at https://github.com/cloudflare/workers-sdk/pull/7614/files#diff-dc9521e48f13c8af9737ee8c555eb22b138b148285e843ce1b4b9bdb3b2e3dd7R1

vicb · 2025-01-06T15:55:43Z

Thanks @petebacondarwin for the review

vicb requested a review from a team as a code owner December 23, 2024 10:50

vicb added the e2e Run wrangler + vite-plugin e2e tests on a PR label Dec 23, 2024

vicb requested a review from petebacondarwin December 23, 2024 10:50

vicb force-pushed the re/7541 branch from a8fb5fa to 50b71f8 Compare December 23, 2024 16:31

vicb force-pushed the re/7541 branch 3 times, most recently from 77d3542 to 7f8eacf Compare January 6, 2025 13:41

feat(wrangler): use unenv bulitin dependency resolution

61c3035

vicb force-pushed the re/7541 branch from 7f8eacf to 61c3035 Compare January 6, 2025 14:28

petebacondarwin reviewed Jan 6, 2025

View reviewed changes

vicb added a commit that referenced this pull request Jan 6, 2025

chore: add details to the changeset of #7625

bfd7df3

petebacondarwin approved these changes Jan 6, 2025

View reviewed changes

vicb merged commit d8fb032 into main Jan 6, 2025

vicb deleted the re/7541 branch January 6, 2025 15:55

workers-devprod mentioned this pull request Jan 6, 2025

Version Packages #7608

Merged

vicb added a commit that referenced this pull request Jan 6, 2025

chore: add details to #7625 changeset (#7678)

6c5306d

This was referenced Jan 15, 2025

chore(wrangler): use the unenv preset from @cloudflare/unenv-preset #7720

Merged

fix(wrangler): use require.resolve to resolve unenv path #7804

Merged

Conversation

vicb commented Dec 23, 2024 • edited by petebacondarwin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot Bot commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions Bot commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

vicb commented Jan 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vicb commented Dec 23, 2024 •

edited by petebacondarwin

Loading

changeset-bot Bot commented Dec 23, 2024 •

edited

Loading

github-actions Bot commented Dec 23, 2024 •

edited

Loading