Support remote and local assets in custom CSS #1372
Conversation
(This includes images and fonts using url().) Using onResolve (as suggested in #786 (comment)) closes #786 supersedes #788
There was a problem hiding this comment.
Nice, this is a useful addition!
Some nits, plus I think there is some plumbing missing: getResolvers should now resolve the local assets that are referenced by local stylesheets. Then I don’t think you’ll need to accumulate the files separately during build, since getResolvers will have done it already. And that would mean that we’d also now be watching these referenced files during preview, so we’d get hot replacement if you edit a local file referenced by a local stylesheet. (Though we’ll also need to add ?sha… for references to local files from local stylesheets as mentioned in the comments.)
src/build.ts
Outdated
| aliases.set(resolveStylesheetPath(root, specifier), alias); | ||
| await effects.writeFile(alias, contents); | ||
| delayedStylesheets.add(specifier); | ||
| for (const file of (await bundleStyles({path: join(root, specifier)})).files) files.add(file); |
There was a problem hiding this comment.
It might be cleaner to have two separate methods (that use the same underlying implementation) since we always call bundleStyles in one of two ways: bundleStyles could return the contents as before, and resolveStyleFiles could return the set of files. But low priority…
src/rollup.ts
Outdated
| build.onResolve({filter: /./}, (args) => { | ||
| if (args.path.endsWith(".css") || args.path.match(/^[#.]/)) return; | ||
| files.add(args.path); | ||
| const path = join("..", aliases?.get(args.path) ?? join("_file", args.path)); |
There was a problem hiding this comment.
Another big advantage of the resolve hook is that bundleStyles won’t have to know about the _file directory — that logic can be supplied by the build command.
And for the preview command, ideally we’d supply a resolve hook that does ?sha=… so that module replacement works when you update a file referenced by a local stylesheet?
There was a problem hiding this comment.
Now we pass the marked URL, but I wasn't able to positively test hot module replacement.
fix a bug where we would rewrite data: urls
|
I think it's close to done? (I'm not sure what you have in mind so that it can be finished.) |
|
Have you pushed your changes? I don’t see any changes to |
|
I changed |
|
We need to parse the stylesheets and resolve/collect all the transitive dependencies. We can pair on it sometime. |
|
I think it works already, see for example https://github.com/observablehq/framework/pull/1372/files#diff-8adbd6b9683bb1d5268aa477247f4c6edf9ea93e5b13397535ff450b4d57ae38 |
|
I’m aware it works, but I think we should implement it by moving it into |
|
Been trying to import self-hosted font assets and stumbled upon this PR. It would be great to be able to define |
|
I think there’s probably a lot more that we want to do here, and I haven’t figured out yet if it makes sense to try to do it all together or if there’s a more piecemeal approach. Specifically, what about inline styles like this? <style type="text/css">
body {
background-image: url("image.png");
}
</style>We could detect the reference to Similarly, given this: <link rel="stylesheet" href="test.css" type="text/css">And the following stylesheet: body {
background-image: url("image.png");
}Presumably we’d want to rewrite And then there’s the question of whether I want to think about this some more. |
Allowing @font-face to import font files that are local to a project would be good practice for possible archiving. |
This includes images and fonts using url().
Using onResolve (as suggested in #786 (comment))
closes #786
supersedes #788
The test above uses a remote
@font-faceand a local background asset (horse.jpg); but any file type is supported (so you can finally have the custom cursors you've been dreaming of).TODO: