-
Notifications
You must be signed in to change notification settings - Fork 31
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
Asset Primitive #106
Asset Primitive #106
Conversation
Thanks for pulling together the details and examples for an RFC so quickly @FredKSchott! Great to see we're getting ahead of the SSR challenges with asset imports with one recommended approach for different file assets 👍 Would there be any issue with using other Vite plugins that also process imports for specific file extensions, like Come to think of it, what would it look like if this was a standalone plugin (either for Astro or Vite) so this was completely opt-in? Maybe even as a great first use case for a tool similar to |
Great question! This actually moves us towards better 3rd party Vite plugin support, since all Vite plugins work through the Vite asset graph which is similar to what this RFC is proposing. There are some things inside of Astro that mean we can't support all existing Vite plugins, but those are outside the scope / unrelated to this RFC.
I think that our job is to create the primitive that all plugins & integrations could build on top of. So to build on this idea, the |
proposals/0000-asset-primitive.md
Outdated
- Works with SSR, where all assets must be known at build-time, with 100% accuracy. | ||
- Works across frameworks (`.jsx`, `.svelte`, `.vue`). Not an `.astro`-only solution. | ||
- Introduces an acceptable amount of boilerplate (as little as possible, or none). | ||
- Can power custom 3rd-party integrations and components. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this power 3rd-party (or even 1st party) integrations? I think that's what's missing most from this proposal.
The only thing from this proposal that really looks different to me from what can be done today is that you can import an SVG without using ?raw
. Is that the only change here, or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarified to be more clear. The "powering" of 3rd party integration was in reference to the fact that this RFC lets an integration author assume a common data structure for certain assets.
For example, if you are build an image component today, Astro provides no standard idea of what an image is. Today, we leave it to the integration to override and dictate what happens when you import an image in an Astro project. For example: https://github.com/ElMassimo/vite-plugin-image-presets
proposals/0000-asset-primitive.md
Outdated
- `Font`: Creates a `<style>` tag with a single `@font-family` definition | ||
- `Image`: Creates a `<img>` tag with `src=` the asset URL | ||
- `Icon`: renders the given `svg` content to HTML. | ||
- In the future, these can be extended to add new features. For example, the Image component can be extended to support image optimizations and automatic [blurhash](https://blurha.sh/) support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the most important part of figuring out assets. I don't think these components can be extended to do those kinds of things, as there are missing primitives around transforming files either during the build or at runtime.
If you want to make this a future proposal I think that's fine, but in that case I don't think the builtin components should be added until that part is ready. A 3rd party should have the same ability to do transformations as a builtin component and I think shipping builtins before that sends a signal to the community that the builtins are going to get special APIs that they won't have access to.
We made this mistake with the Markdown component (giving special APIs) and haven't yet been able to undo it. Once something is implemented high-level there is less of an incentive to ever pull out the primitives, so I'd be disappointed if we did this again with asset components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm aligned on this. The built-in components were included to make it clear how you would use these assets, but I can illustrate this without actually proposing that we build them before we are ready.
I added comments but will summarize my feedback here:
|
Thanks for the great feedback @matthewp, this is super helpful! I'm still going through it to make sure everything is addressed. Update: @matthewp all comments addressed. I tried to resolve things that I answered by improving the RFC to be more clear, but if there are still concerns even with the answers given please feel free to unresolve. I added a note about the fact that this RFC intentionally prioritizes the inline SVG use-case. Someone else may be able to provide better resources, but here are two that I found useful: https://css-tricks.com/svg-loader-a-different-way-to-work-with-external-svg/ (gets at why usability is better) & https://salience.co.uk/insight/magazine/inline-svg-page-speed-seo/#why-use (gets at why perf is better). |
I have some concerns with this proposal, which stem from its purpose, which I read to be "a consistent strategy and low-level primitive to handle assets". First, we have a consistent way to handle assets that should not require an author to use lower-level primitive: <style>
@font-face {
font-family: "Comforter";
src: url("/src/fonts/Comforter.woff2");
}
</style> <link rel="preload" href="/src/fonts/Comforter.woff2" as="font" />
<link rel="stylesheet" href="./relative-to-component-style.css" />
<script src="./relative-to-component-client-script.js"></script> I am aware that other RFCs removed some of these ways. That was a mistake this proposal should not embrace. Second, we have a consistent way to emit chunks and assets — These existing foundations should work, and this RFC should build upon them before attempting to replace them. We have page authors who would like the former bit to work, and we have plugin authors who would like the later bit to work. For Page authors: We should let HTML work™ if we can. My mind has changed regarding relative paths in AstroHTML files, especially after watching and listening to users try to use AstroHTML files alongside assets. They intuit For Plugin Authors: We should like plugin APIs work™ if we can. We should embrace a plugin API for attaching assets. This proposal makes no mention of the existing 'Vite way'. I think Astro should embrace this way, paying closer attention to these plugin primitives. There are 2 reasons I write 'pay closer attention':
<script type="module">
import oh_was_that_private from '/etc/hosts?raw'
console.log(oh_was_that_private)
</script> I suggest we get these issues with the existing functionality sorted out first, because I believe they would provide most of what this proposal seeks to provide, without inventing something new that would be used to do the same thing. P.S. I really like some of the ideas in this proposal, and I have a separate comment about this. However, what I like about this proposal may not be related to the primary purpose of this RFC. |
@FredKSchott Understandable! I spent too long side-eyeing the In fact, could these primitives all be one Vite plugin that I can add and remove from my Astro config? import astroAssetsPlugin from 'astro/config'
export default /** @type {import('astro').AstroUserConfig} */ ({
...
vite: {
plugins: [astroAssetsPlugin()],
},
}); ☝️ Might be worth auto-applying for Astro projects specifically. But if this were all bundled into a Vite plugin, it could be used across the open source ecosystem as well 😁 |
And yes yes I know I like the idea of a <render:svg local:src="/path/to/vector.svg" /> I could even imagine custom renderers to handle complex tasks. For instance, image optimization: <render:picture local:src="/path/to/unoptimized.png" /> Am I bullying |
I think this RFC should be updated to embrace the other ways we can statically analyze code that do not conflict with other RFCs. Specifically, we can already statically analyze CSS. This has a potentially large impact on the RFC, because it means don’t need a fonts component to add a <style>
body {
background-color: green;
color: white;
font: 200%/1.5 'Comforter', monospace;
margin: 5%;
}
</style> That can be enough to generate a For plugin authors wishing to pair static analysis with asset injection, it would be would be nice to have a low-level API for adding assets in plugins. This low-level API should allow plugin authors to statically inject things into // PostCSS Plugin with its own `getFontFamilyName`, `getPathToFont`, & `generateFontFaceRule` methods.
{
Declaration(decl) {
if (isFontDeclaration(decl.prop)) {
const fontFamily = getFontFamilyName(decl.value)
const src = getPathToFont(fontFamily)
const fontFaceRule = generateFontFaceRule({ fontFamily, src })
bikeshed_low_level_asset_api.add(fontSrc)
bikeshed_low_level_asset_api.injectCSS(fontFaceRule)
}
}
} With something like Current Behavior: import style from './style.css'
console.log(style) // currently prints the CSS and also changes the page HTML With a Low-Level API: import style from './style.css'
console.log(style) // just prints the CSS
bikeshed_low_level_asset_api.injectCSS(style) // changes the page HTML |
For HTML, if we can support Similarly, we should support |
What I love about this proposal is the idea that Since this RFC does not include a detailed design of
|
There is many things I like about the spirit of this proposal. I have thought long and hard over my response, there is alot of merits to this, however there was a couple of issues that I wish for was available to component authors. |
RFC Call: Thanks everyone for the feedback! Some notes to make sure that history is recorded:
|
We agreed that, in any kind of JS (which includes JS files, JS derivative files like JSX or TS, etc, and any blocks of JS like the front-matter and expressions of Astro files), it is acceptable to import an asset. We did not attempt consensus regarding what an asset specifically is, but it was well understood to be images and fonts, with PNG, SVG, and WOFF files specifically called out as examples. We agreed there would be no limits on the path that an asset could be imported from at this time. It was noted that this is and has been the behavior in Astro. This behavior is documented. There were issues raised about why the proposal was limiting itself to JS files, as many individuals did not want to see Astro prioritize a JS-first experience. We did not attempt consensus on what the import would return. There was informal consensus that imports should probably return the same (not yet defined) thing, whether they were imports of relative assets, imports of external package assets, or imports from within external packages. However, there were also concerns raised about this behavior and how it may collide with packages relying on different import behavior. The author intends to prototype and continue the discussion here, and to reach further consensus again at a later date. |
Closing this as something that I don't plan to pick back up personally until after v1.0. As of right now we are relying on Vite defaults to power our ESM import statements. I would love to see more user-land exploration around an |
Summary
A consistent asset strategy and low-level primitive to handle assets (Icons, Fonts, Images, etc.) in Astro. Useful for users, integration authors, and to power future, more high-level features.
Links