Incorrect paths after asset compile

svoop commented 8 months ago

From the guides, it’s a feature that e.g. /app/assets/fonts/hack/hack-regular.woff2 is copied to /public/assets/hack/hack-regular.woff2 during asset compile obviously dropping the first hierarchy fonts/.

However, this is a problem when you reference fonts, background images etc from another stylesheet. I’ve created a simple test app to illustrate:

https://github.com/svoop/testli/commits/main/

After bootstrapping, I’ve just added a fonts directory, added the stylesheet for it and added it via the app.js entrypoint:

https://github.com/svoop/testli/commit/7e6cad8f54ec054f7f711048af692a1e5b991e0c

Running hanami assets compile works fine and produces the following assets in public/:

https://github.com/svoop/testli/commit/81a26154b10f11d8f7654a9fdc7ac8e65b3e6fac

As expected the hack.woff2 is created without fonts/ in its path:

https://github.com/svoop/testli/blob/81a26154b10f11d8f7654a9fdc7ac8e65b3e6fac/public/assets/assets.json#L12

That’s not the case in the compiled app.css which contains the fonts/ path segment.

https://github.com/svoop/testli/blob/81a26154b10f11d8f7654a9fdc7ac8e65b3e6fac/public/assets/app-QT5OCCA4.css#L1

Also, possibly unrelated, the path in the compiled app.css descends below the public/ webroot for static assets with ../../ which doesn’t seem right.

svoop commented 8 months ago

@timriley answered:

I’ve just replicated this here locally too, and IMO this is definitely a bug.

Per esbuild’s CSS docs, we do configure a loader for font files, but this clearly isn’t doing the job we need it to do.

To test this locally, I put a file in app/assets/fonts/ and referred to it from a CSS file just like your example above.

Then I edited my config/assets.js to increase the level of logging from esbuild:

await assets.run({
  esbuildOptionsFn: (args, esbuildOptions) => {
    esbuildOptions.logLevel = "verbose";

    return esbuildOptions;
  },
});

And then when I ran hanami assets compile, I saw this relevant bit of output:

[two_one] ⬥ [VERBOSE] Resolving import "../fonts/comic-mono.ttf" in directory "/Users/tim/Source/hanami/scratch/two_one/app/assets/css" of type "url-token"
[two_one]
[two_one]   Checking "../fonts/comic-mono.ttf" against the external pattern "/Users/tim/Source/hanami/scratch/two_one/app/assets/images/*"
[two_one]   Checking "../fonts/comic-mono.ttf" against the external pattern "/Users/tim/Source/hanami/scratch/two_one/app/assets/fonts/*"
[two_one]   Read 1 entry for directory "/Users/tim/Source/hanami/scratch/two_one/app/assets/css"
[two_one]   Checking "/Users/tim/Source/hanami/scratch/two_one/app/assets/fonts/comic-mono.ttf" against the external pattern "/Users/tim/Source/hanami/scratch/two_one/app/assets/images/*"
[two_one]   Checking "/Users/tim/Source/hanami/scratch/two_one/app/assets/fonts/comic-mono.ttf" against the external pattern "/Users/tim/Source/hanami/scratch/two_one/app/assets/fonts/*"
[two_one]   The path "/Users/tim/Source/hanami/scratch/two_one/app/assets/fonts/comic-mono.ttf" was marked as external by the user
[two_one]   Read 5 entries for directory "/Users/tim/Source/hanami/scratch/two_one/app/assets"
[two_one]   Read 1 entry for directory "/Users/tim/Source/hanami/scratch/two_one/app/assets/fonts"
[two_one]   Primary path is "/Users/tim/Source/hanami/scratch/two_one/app/assets/fonts/comic-mono.ttf" in namespace "file"

See here how it’s mentioning that the font was “marked as external?” This is an esbuild feature that can indicate files to exclude from the build. And in assets-js, we actually mark all directories aside from css/ js/ as external. This is what allows us to then handle the files in those directories separately (such as images), and then later copy them across into the compiled assets directory as part of our hanami-assets esbuild plugin.

However, what we’re discovering is that this also means that files in those external directories cannot be referenced from within JS or CSS files and have their file references appropriately updated for the resulting bundle.

To test this for sure, I moved the font directly inside the app/assets/css/ directory, and referred to it there:

@font-face {
  font-family: "comic-mono";
  src: url("./comic-mono.ttf");
}

And then when I ran hanami assets compile again, we see more encouraging output:

[two_one] ⬥ [VERBOSE] Resolving import "./comic-mono.ttf" in directory "/Users/tim/Source/hanami/scratch/two_one/app/assets/css" of type "url-token"
[two_one]
[two_one]   Checking "./comic-mono.ttf" against the external pattern "/Users/tim/Source/hanami/scratch/two_one/app/assets/images/*"
[two_one]   Checking "./comic-mono.ttf" against the external pattern "/Users/tim/Source/hanami/scratch/two_one/app/assets/fonts/*"
[two_one]   Read 2 entries for directory "/Users/tim/Source/hanami/scratch/two_one/app/assets/css"
[two_one]   Checking "/Users/tim/Source/hanami/scratch/two_one/app/assets/css/comic-mono.ttf" against the external pattern "/Users/tim/Source/hanami/scratch/two_one/app/assets/images/*"
[two_one]   Checking "/Users/tim/Source/hanami/scratch/two_one/app/assets/css/comic-mono.ttf" against the external pattern "/Users/tim/Source/hanami/scratch/two_one/app/assets/fonts/*"
[two_one]   Read 2 entries for directory "/Users/tim/Source/hanami/scratch/two_one/app/assets/css"
[two_one]   No "browser" map found in directory "/Users/tim/Source/hanami/scratch/two_one/app/assets/css"
[two_one]   Attempting to load "/Users/tim/Source/hanami/scratch/two_one/app/assets/css/comic-mono.ttf" as a file
[two_one]     Checking for file "comic-mono.ttf"
[two_one]     Found file "comic-mono.ttf"
[two_one]   Checking "/Users/tim/Source/hanami/scratch/two_one/app/assets/css/comic-mono.ttf" against the external pattern "/Users/tim/Source/hanami/scratch/two_one/app/assets/images/*"
[two_one]   Checking "/Users/tim/Source/hanami/scratch/two_one/app/assets/css/comic-mono.ttf" against the external pattern "/Users/tim/Source/hanami/scratch/two_one/app/assets/fonts/*"
[two_one]   Read 2 entries for directory "/Users/tim/Source/hanami/scratch/two_one/app/assets/css"
[two_one]   Primary path is "/Users/tim/Source/hanami/scratch/two_one/app/assets/css/comic-mono.ttf" in namespace "file"

Note above that it no longer says “marked as external by the user”, and instead it shows that it’s using the file loader for the font file in the “attempting to load … as a file” line.

And then if I inspect the compiled CSS, things look good too, with it referring to a properly hashed file, and that file actually existing alongside it inside public/assets/:

@font-face{font-family:comic-mono;src:url("./comic-mono-5YVBLHHO.ttf")}

So what’s the takeaway here?

We have a bug where files outside of the css/ and js/ assets directories cannot be directly referenced within CSS and JS files and properly included in the bundle
This is because we make those directories as “external”, which means esbuild doesn’t process those files
I wonder if we even need that external config at all; what happens if we just take it away? We do currently use it inside our esbuild plugin to get the list of dirs whose assets we should manually copy , but what if we just did a direct filesystem crawl at that point (the one we currently do here), instead of using build.initialOptions.external? That might still give us the overall behaviour we want while avoiding this bug for files directly referenced from CSS/JS.

timriley commented 8 months ago

@svoop Thanks for copying this over. FYI, I've made a start on this in https://github.com/hanami/assets-js/pull/26.

It's already working now, but there's a bit of tidying left to do (right now it creates duplicate copies of the files in the non-js/css directories). If you wanted to give it a try with your apps, please feel free!

svoop commented 8 months ago

@timriley Cool, thanks for working on this! (It's a little embarrassing, but TypeScript really isn't my turf.)

It looks much better already, however, the non-CSS/JS assets are copied over twice, once into/public/assets and once into the subdir therein. Here's my simple test app with one font in /app/assets/fonts/hack/hack-regular.woff2:

https://github.com/svoop/testli/tree/pr26-test1/public/assets

https://github.com/svoop/testli/commit/c850e8843b72d1b47a8715dd7130f25fd4c60a4c

I'm wondering: What is the idea and benefit of flattening the directory hierarchies when the non-CSS/JS assets are copied over to /public/assets? Wouldn't it be easier and less surprising to keep the directory structure?

(Just saw, you mention these dupes as a TODO on the PR already.)

krzykamil commented 8 months ago

I just noticed that the discourse has been moved to github issue. I've checked @timriley branch and tried adding the duplicates fix, this is the result on @svoop test app. I will test this further on my app and post a PR to your PR Tim for you to review.

This does not keep the subdirectories structure. I am not sure what approach is prefered, but this seemed to be intentional so I left it as is.

EDIT: Results from my app, where slice has its assets, with fonts nested in subfolder. Seems alright

krzykamil commented 8 months ago

https://github.com/hanami/assets-js/pull/27/files I posted a PR that references Tims PR

hanami / assets-js

Incorrect paths after asset compile #24