feat: support npm resolution for file imports

[Cherry]

Fixes #4098.

What this PR solves / how to test:

Previously when importing static files (such as wasm) from an npm package, you had to path directly to it, such as :

import wasm from '../../node_modules/svg2png-wasm/svg2png_wasm_bg.wasm'

While this works file, the DX isn't ideal, so this PR adds proper package resolution for these statements, allowing you to do:

import wasm from 'svg2png-wasm/svg2png_wasm_bg.wasm'

This uses resolve.exports to look at and analyse the package's exports field, and resolve to the applicable file. This does add an additional dependency, but this is a very common one used by so much build tooling including vite, jest, svelte, and more.

If for any reason this fails, it falls back to the previous behaviour and will just do a path reference, and then end up throwing an ENOENT as per previous functionality.

Notes:

• My detection for "is this possibly an npm package" is very dumb right now, checking for presence of / and not starting with .. It seems to work remarkebly well in practice across the many projects I've tested this against, but I'm happy to improve this if someone has a cleaner idea.
• While I've added tests for the npm package name extraction to ensure it handles scoped packages, I've not added any specific tests for handling of the imports. To test that, it'd need a fixture with a real node_modules folder, and dummy package, which I don't think there's any prior art for in the repo today. Happy to add these if someone can guide me on the best way to approach this.

Author has included the following, where applicable:

• Tests
• Changeset (Changeset guidelines)

Reviewer is to perform the following, as applicable:

• Checked for inclusion of relevant tests
• Checked for inclusion of a relevant changeset
• Checked for creation of associated docs updates
• Manually pulled down the changes and spot-tested

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

[changeset-bot]

🦋 Changeset detected

Latest commit: 9977e9c

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

This PR includes changesets to release 1 package

Name	Type
wrangler	Minor

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]

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/6627685921/npm-package-wrangler-4135

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

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

Or you can use npx with this latest build directly:

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

Additional artifacts:

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

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

---

wrangler@3.14.0 includes the following runtime dependencies:

Package	Constraint	Resolved
miniflare	3.20231016.0	3.20231016.0
workerd	1.20231016.0	1.20231016.0
workerd --version	1.20231016.0	2023-10-16

|

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

[codecov]

Codecov Report

Merging #4135 (9977e9c) into main (0cac2c4) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 59.09%.

@@           Coverage Diff           @@
##             main    #4135   +/-   ##
=======================================
  Coverage   75.36%   75.36%           
=======================================
  Files         223      223           
  Lines       12259    12280   +21     
  Branches     3171     3177    +6     
=======================================
+ Hits         9239     9255   +16     
- Misses       3020     3025    +5     

Files	Coverage Δ
packages/wrangler/src/deployment-bundle/bundle.ts	90.82% <100.00%> (+0.08%)	⬆️
...rangler/src/deployment-bundle/module-collection.ts	87.20% <57.14%> (-6.14%)	⬇️

... and 3 files with indirect coverage changes

[petebacondarwin]

Can this be done more simply using the node.js require.resolve() function?

Disclaimer: I haven't checked this at all!

For example something like:

let filePath = path.join(args.resolveDir, args.path);
if (!path.isAbsolute(args.path) && !args.path.startsWith(".")) {
	try {
	filePath = require.resolve(args.path)
} catch {
	// Do nothing
}

Note that your current checks for package like paths would break on Windows.

[Cherry]

Can this be done more simply using the node.js require.resolve() function?

Unfortunately not from my testing. Running require.resolve with args.path as svg2png-wasm/svg2png_wasm_bg.wasm results in Error: Cannot find module 'svg2png-wasm/svg2png_wasm_bg.wasm' with code MODULE_NOT_FOUND. I don't know exactly why this is, but I'd suspect it's something to do with CJS vs ESM interop, and newer exports mapping.

Note that your current checks for package like paths would break on Windows.

Can you expand here? My checks are purely checking for an included /, and not starting with ..

If I try and import like svg2png-wasm\\svg2png_wasm_bg.wasm, this plugin doesn't even run, and I get a generic esbuild error: No loader is configured for ".wasm" files: ../../../../example-wasm/node_modules/svg2png-wasm/svg2png_wasm_bg.wasm. I suspect this is because this plugin only runs on the specified filter, which looks something like this, controlled way higher in wrangler's rules stuff:

globs: ["**/*.wasm", "**/*.wasm?module"]

Supporting more windows orientated backslashes seems like a general issue here, and not really in the scope of this PR if I'm understanding correctly. It's also extremely uncommon, even on Windows from my experience, to import using backslashes in the path, especially with npm modules. This would also explain why no one has reported it as a bug.

[petebacondarwin]

Supporting more windows orientated backslashes seems like a general issue here, and not really in the scope of this PR if I'm understanding correctly. It's also extremely uncommon, even on Windows from my experience, to import using backslashes in the path, especially with npm modules. This would also explain why no one has reported it as a bug.

Yeah that is a good spot. The rules themselves wouldn't match in the first place.

[petebacondarwin]

Could do with another review from a Wrangler developer, though.

[Cherry]

All of the E2E test failures look unrelated, failing due to AssertionError: Please provide a CLOUDFLARE_ACCOUNT_ID as an environment variable

[petebacondarwin]

All of the E2E test failures look unrelated, failing due to AssertionError: Please provide a CLOUDFLARE_ACCOUNT_ID as an environment variable

Yeah that's because they don't run on collaborator forks only on the Cloudflare owned fork. No worries.

[penalosa]

This is looking great! I would like to see some tests of this behaviour: perhaps similarly to the worker-app example, which imports from another fixture, you could write a fixture which imports a wasm file from another fixture?

[Cherry]

Thanks! I've added two test fixtures now - one just exporting a simple multiply.wasm file (stolen from another fixture), and then the other importing it, using it, and a really basic test that it's reachable (effectively just didn't crash and burn during build). In doing so, I found some small issues with how I was using resolveDir vs process.cwd() so got that all fixed up too.

Let me know if there's any further changes needed here.

[Cherry]

The failing tests seem to be in external-service-bindings-app:test:ci, and unrelated to this PR.

EDIT: merged from upstream - seems this was fixed in a recent commit.

EDIT2: seems the service-bindings-app tests are flaky. They failed in the most recent version commit too; https://github.com/cloudflare/workers-sdk/actions/runs/6579250313/job/17874703559

[Cherry]

Updated from main which includes a fix for the unrelated failing test. This should all be green now. 🥳

[Cherry]

Nevermind, something in service-bindings-app is still flaking and throwing a v8 error. Completely unrelated to this PR, but preventing it from going green.