Skip to content
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: avoid esbuild warning when running dev/bundle #5999

Closed
wants to merge 1 commit into from

Conversation

threepointone
Copy link
Contributor

I've been experimenting with esbuild 0.21.4 with wrangler. It's mostly been fine. But I get this warning every time

▲ [WARNING] Import "__INJECT_FOR_TESTING_WRANGLER_MIDDLEWARE__" will always be undefined because there is no matching export in "src/index.ts" [import-is-undefined]

    .wrangler/tmp/bundle-Z3YXTd/middleware-insertion-facade.js:8:23:
      8 │ .....(OTHER_EXPORTS.__INJECT_FOR_TESTING_WRANGLER_MIDDLEWARE__ ?? []),
        ╵

This is because [email protected] enabled a warning by default whenever an undefined import is accessed on an imports object. However we abuse imports to inject stuff in middleware.test.ts. We should probably fix that with a better solution, but an immediate fix to this warning is to make that access not be statically analyzable. This patch does so by defining the export name as a regular variable and using that when reading from it.

@threepointone threepointone requested a review from a team as a code owner June 8, 2024 18:28
Copy link

changeset-bot bot commented Jun 8, 2024

🦋 Changeset detected

Latest commit: 66d690b

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

Copy link
Contributor

github-actions bot commented Jun 8, 2024

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/9430830237/npm-package-wrangler-5999

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9430830237/npm-package-wrangler-5999 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9430830237/npm-package-create-cloudflare-5999 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9430830237/npm-package-cloudflare-kv-asset-handler-5999
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9430830237/npm-package-miniflare-5999
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9430830237/npm-package-cloudflare-pages-shared-5999
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9430830237/npm-package-cloudflare-vitest-pool-workers-5999

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


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240605.0
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.

I've been experimenting with esbuild 0.21.4 with wrangler. It's mostly been fine. But I get this warning every time

```
▲ [WARNING] Import "__INJECT_FOR_TESTING_WRANGLER_MIDDLEWARE__" will always be undefined because there is no matching export in "src/index.ts" [import-is-undefined]

    .wrangler/tmp/bundle-Z3YXTd/middleware-insertion-facade.js:8:23:
      8 │ .....(OTHER_EXPORTS.__INJECT_FOR_TESTING_WRANGLER_MIDDLEWARE__ ?? []),
        ╵
```

This is because [email protected] enabled a warning by default whenever an undefined import is accessed on an imports object. However we abuse imports to inject stuff in `middleware.test.ts`. We should probably fix that with a better solution, but an immediate fix to this warning is to make that access not be statically analyzable. This patch does so by defining the export name as a regular variable and using that when reading from it.
@@ -861,7 +861,19 @@ describe("middleware", () => {
.replace(/\/\/ .*/g, "")
.trim()
).toMatchInlineSnapshot(`
"var src_default = {
"var __defProp = Object.defineProperty;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of note, it adds some extra code for reading that dynamically. which is why we need a proper fix later so we don't ship any extra code. but this should be fine for now.

@threepointone
Copy link
Contributor Author

Alternately, we should delete middleware.test.ts, it's testing an implementation detail that's explicitly added for the test, which is weird. The test should be for actual middleware.

@penalosa penalosa mentioned this pull request Jun 11, 2024
12 tasks
@petebacondarwin
Copy link
Contributor

Let get #6006 green and land that since it also avoids adding code to production bundles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants