Fix broken image import in bundled build.

[cain-wang]

The line I removed set resolveProxyImports to false for bundled builds and cause image and json files to be imported directly instead of through proxy imports.

// build/dist/App.js
import logo from "./logo.svg";

This change fixes:

[BUG] sowpack bundle build generate import statement for images and json files #3109
#3109

Changes

This change prevents optimize.bundle to automatically set buildOptions.resolveProxyImports to false.

Testing

Tested bundled builds with @snowpack/app-template-react and the the proxy imports is properly resolved to:

// build/dist/logo.svg.proxy.js
var logo_svg_proxy_default = "/dist/logo.svg";

Docs

Bug fix only

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/DjzrBu7ghVr8WbdkesrF3CFZmuRn
✅ Preview: https://snowpack-git-fork-cain-wang-proxy-pikapkg.vercel.app

[matthewp]

@lukejacksonn Think you can add a test for this one?

[lukejacksonn]

Hey @cain-wang for me removing this line I see this in the output like you:

var logo_svg_proxy_default = "/dist/logo.svg";

But if you look at the tests you can see that it has the adverse effect of producing an extra file /dist/App.css and I am not sure as to why this happens yet. Any clues?

Array [
  +   "dist/App.css",
      "dist/index.css",
      "dist/index.js",
      "dist/index.js.map",
      "dist/logo.svg",
      "index.html",
]

Either way I have updated the tests and added a check to see that non-js files are proxied and not directly imported. Let me know if you think this sufficiently covers the failure/fix.

[drwpow]

I know this resolves your specific issue, but this is a really big change to bundled output.

config.buildOptions.resolveProxyImports = !config.optimize?.bundle;

The reason for this specific line of code is: when you’re using Snowpack, and the Snowpack dev server, things like images, CSS files, and basically everything non-JS can’t be import-ed. So we create these intermediary proxy files so the browser can understand them.

But when we’re optimizing, either through esbuild or webpack, the proxy files throw a wrench into how those bundlers work. The bundlers need the original imports so they can work their magic, and optimize better than the proxy files can allow. According to the docs, esbuild does support using image imports, so I’d rather hone in on what specifically isn’t working with this output for esbuild, rather than change all behavior for all files in all bundlers.

[drwpow]

This is a breaking change that affects all files and all bundlers. To resolve the issue of image resolution with esbuild, this PR should target that specific scenario of .svg + esbuild and only that scenario.

[cain-wang]

Thanks for taking a look at this issue! I threw this pull request out to illustrate where the code affects the bundling behavior. It would be great to have a property fix for this. As long as we fix the direct import of non-js files, this should fix our build!

Let me also take a look at what @drwpow suggested and see how it works with esbuild and why it fails in our system.

Hey @cain-wang for me removing this line I see this in the output like you:

var logo_svg_proxy_default = "/dist/logo.svg";

But if you look at the tests you can see that it has the adverse effect of producing an extra file /dist/App.css and I am not sure as to why this happens yet. Any clues?

Array [
  +   "dist/App.css",
      "dist/index.css",
      "dist/index.js",
      "dist/index.js.map",
      "dist/logo.svg",
      "index.html",
]

Either way I have updated the tests and added a check to see that non-js files are proxied and not directly imported. Let me know if you think this sufficiently covers the failure/fix.

[cain-wang]

Thanks @drwpow for the pointer! I dug more into the code and it seems Snowpack esbuild plugin may not handle the non-js files as I thought:

When snowpack pushes the esbuildPlugin here, it only adds ['.mjs', '.jsx', '.ts', '.tsx'] as input.

And inside the plugin-esbuild.ts, its getLoader function mostly returns the file extension as the loader, which would not able to translate from .svg to 'dataurl' loader.

Any suggestion if we should fix the esbuild plugin instead?

[drwpow]

Thanks @drwpow for the pointer! I dug more into the code and it seems Snowpack esbuild plugin may not handle the non-js files as I thought:

When snowpack pushes the esbuildPlugin here, it only adds ['.mjs', '.jsx', '.ts', '.tsx'] as input.

And inside the plugin-esbuild.ts, its getLoader function mostly returns the file extension as the loader, which would not able to translate from .svg to 'dataurl' loader.

Any suggestion if we should fix the esbuild plugin instead?

This is more of an internal thing that’s not documented outside the code, but it’s helpful to think of Snowpack in 2 stages:

1. Build: this is Snowpack core. This takes everything like JSX, Vue, Svelte, and even importing files in JS and build them for the browser. This happens both in snowpack dev and snowpack build
2. Optimize: this only happens on snowpack build if you enable bundling.

In both stages, we run esbuild, but the two don’t talk to each other. The first run in the Build step is for handling TypeScript and JSX natively without needing plugins for those and uses esbuild’s Transform API. This is that esbuild plugin you mentioned—it happens in build and is actually unrelated to bundling.

However, this PR’s issue deals only with the Optimize esbuild run, which happens in a completely different stage, and uses esbuild’s Build API by contrast. This second stage is opt-in, and where esbuild isn’t handling SVG properly. So we basically need to change what we’re handing off to esbuild here, without disrupting the build.

Hopefully this provides a little more context into the issue! Code-wise, we should only be dealing with src/build/optimize.ts for the most part to tackle this issue. And we should try and solve only the SVG issue without changing the bundled output for everything else.

[drwpow]

Unfortunately I’ll have to close this PR, as though this may fix one problem it results in myriad others and will be a worse experience.

For those watching this thread, we’ll be spending time telling a clearer bundling story for Snowpack, because we know that the existing plugins aren’t usable for many people. I don’t have anything more to say in that regard, other than some exciting changes are coming! And we appreciate your patience.