-
Notifications
You must be signed in to change notification settings - Fork 759
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
refactor: use require.resolve
and remove resolve
dependency
#6752
Conversation
🦋 Changeset detectedLatest commit: 5935b3b 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/12157810991/npm-package-wrangler-6752 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6752/npm-package-wrangler-6752 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12157810991/npm-package-wrangler-6752 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12157810991/npm-package-create-cloudflare-6752 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12157810991/npm-package-cloudflare-kv-asset-handler-6752 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12157810991/npm-package-miniflare-6752 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12157810991/npm-package-cloudflare-pages-shared-6752 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12157810991/npm-package-cloudflare-vitest-pool-workers-6752 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12157810991/npm-package-cloudflare-workers-editor-shared-6752 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12157810991/npm-package-cloudflare-workers-shared-6752 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12157810991/npm-package-cloudflare-workflows-shared-6752 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
I had to remove a test to fix the fail. Looks like it's because the test was importing a non-exported entrypoint here:
It worked with the Technically it could be a breaking change that it now respects the |
@bluwy Thanks for this contribution! Given it requires removing a test I don't think we'd want to merge it as-is—we do want to preserve that test's behaviour, if possible |
Alright I'll close this for now then, but I think it's something worth revisiting again in a breaking version to respect the |
What this PR solves / how to test
Fixes N/A
It looks like we can simplify the resolve using
require.resolve
, similar to how other parts of the codebase that also does so:workers-sdk/packages/vitest-pool-workers/src/pool/module-fallback.ts
Line 292 in 0deb42b
workers-sdk/packages/wrangler/src/deployment-bundle/bundle.ts
Lines 332 to 337 in 0deb42b
This allows the package to drop the
resolve
dependency, which isn't used anywhere else in thewrangler
prod dependency graph.Author has addressed the following