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

Asset Primitive #106

Closed
wants to merge 14 commits into from
Closed

Asset Primitive #106

wants to merge 14 commits into from

Conversation

FredKSchott
Copy link
Member

@FredKSchott FredKSchott commented Feb 9, 2022

  • Start Date: 2022-02-08
  • Status: Draft

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.

---
import imageBook from '../assets/book.png';
import iconCookie from '../assets/cookie.svg';
import robotoFont400 from '../assets/fonts/roboto-slab-v22-latin-regular.woff2';
import {Image, Icon, Font} from '../src/my-asset-components/index.js';
---
<html>
  <head>
    <Font use={robotoFont400} family="Roboto Slab" />
    <style>
      body { font-family: 'Roboto Slab'; }
    </style>
  </head>
  <body>
    <p>Hello, <strong>world!</strong></p>
    <Icon use={iconCookie} />
    <Image use={imageBook} />
  </body>
</html>

Links

@tony-sull
Copy link

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 vite-imagetools?

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 astro add @astrojs/images 👀?

@FredKSchott
Copy link
Member Author

Would there be any issue with using other Vite plugins that also process imports for specific file extensions, like vite-imagetools?

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.

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 astro add @astrojs/images 👀?

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 ImageAsset and SvgAsset concepts need to be built into Astro core for consistency for everyone to build on top of. BUT, anyone could build their own 3rd party Icon, Font and Image components if they'd like, using the same primitives.

- 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.
Copy link
Contributor

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?

Copy link
Member Author

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

- `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.
Copy link
Contributor

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.

Copy link
Member Author

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.

@matthewp
Copy link
Contributor

I added comments but will summarize my feedback here:

  • I think this proposal is mostly about changing how certain file types are resolved, specifically .svg and perhaps .png, .jpg, and others. But the specifics of what will change are missing, so I think those should be given full detail.
  • I think it's premature to add builtins that don't do anything extra until the primitives exist that would allow them to do the extra things we might want them to do (like image optimizations).
  • I think a big part of this proposal is around the opinion that inlining SVGs is the right thing to do for icons. If this is the case, can you add links to articles that make the case that they should be inlined? I think having some "best practices" research to accompany the proposal would strengthen it.
  • What about non-icon SVGs? How does this proposal account for adding an SVG that's just an image? Would you pass it to <Image> (i don't think so given the explanation of how Image is set to currently work).

@FredKSchott
Copy link
Member Author

FredKSchott commented Feb 14, 2022

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).

@jonathantneal
Copy link
Contributor

jonathantneal commented Feb 15, 2022

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 — this.emitFile — which comes from Vite (and ultimately Rollup).

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 <img src> and <link href> as resolving source-relative first, otherwise as-is (which would reference public if the asset is present). This proposal moves further from that intuition and closer to NextJS style import all-the-things. Leaning on HTML and CSS is part of what makes Astro a better alternative.

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':

  1. Because this.emitFile was difficult to work with in Astro before, and is now extremely difficult or impossible to work with behind the experimental static build. Was this plugin API known of? Was it being tested? If it cannot be used, can this be highlighted in this proposal?
  2. Because dog-fooding is important. For instance, import still utilizes Vite’s relative import powers, which allows me to do this in client-side code:
<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.

@ghost
Copy link

ghost commented Feb 15, 2022

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).

@FredKSchott Understandable! I spent too long side-eyeing the astro/components note in particular. Sounds like this RFC is just establishing 1 thing: what the heck to SVGs, PNGs, fonts, etc look like when imported via ESM? I'd love for Astro to define all of these out-of-the-box instead of using scattered Vite plugins.

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 😁

@ghost
Copy link

ghost commented Feb 15, 2022

And yes yes I know astro/components is out of scope, but I did want to mention: wouldn't inline SVGs in particular be better served by the render element I've heard floating around?

I like the idea of a <render:md> replacing the existing <Markdown> component. Maybe something similar could be done for SVGs?

<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 render into a sort of 11ty shortcode?? Maybe. Is this still out of scope for this RFC? Absolutely. But it addresses a very real concern @jonathantneal raised: treating ESM-all-the-things as a best practice may not work for everyone. I'm all for defining the ESM standard here for React, Vue, and Svelte to have a predictable format to work with. But when using Astro the templating language... well, I don't want to import all my assets up-front via ESM. I just wanna say local:src= from my HTML. And for those coming from the Jekyll, Hugo, or 11ty spaces, an HTML-y helper like render would work much better for that mindset. Just my 2 cents!

@jonathantneal
Copy link
Contributor

jonathantneal commented Feb 15, 2022

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 @font-face rule. It might not even be the most intuitive way for authors to automatically inject CSS @font-face rules.

<style>
body {
  background-color: green;
  color: white;
  font: 200%/1.5 'Comforter', monospace;
  margin: 5%;
}
</style>

That can be enough to generate a @font-face rule. Try it, because it works.


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 <head>.

// 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 injectCSS, plugin authors could even write plugins to override the current (weird, IMHO) behavior of CSS files in Astro.

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

@jonathantneal
Copy link
Contributor

jonathantneal commented Feb 15, 2022

For HTML, if we can support <img class="friend" local:src="kitten.jpg" /> while knowing how React reacts to class, then we should support <img class="friend" src="kitten.jpg" /> while knowing how React would handle src differently. No namespace attributes necessary.

Similarly, we should support ![friend](kitten.jpg). In fact, if we limited this behavior to the project directory (as I mentioned previously then writing ![friend](/src/assets/kitten.jpg) would work the same in Markdown files rendered in both Astro or GitHub.

@jonathantneal
Copy link
Contributor

jonathantneal commented Feb 15, 2022

What I love about this proposal is the idea that import would return objects representing their entries, rather than as strings of the import path. While the current proposal restricts woff2 to a string, I would suggest it return an object and work consistently for all media assets.

Since this RFC does not include a detailed design of ImageAsset, I propose the RFC be updated so that:

  1. Unknown types return as File or FileSystemFileEntry, which may provide most of what is desired.
  2. Known types follow the MIME Sniffing Living Standard. At least the relevant mimes to this RFC, which are:

    An image MIME type is a MIME type whose type is "image".

    An audio or video MIME type is any MIME type whose type is "audio" or "video", or whose essence is "application/ogg".

    A font MIME type is any MIME type whose type is "font", or whose essence is one of the following: [RFC8081]

    application/font-cff
    application/font-off
    application/font-sfnt
    application/font-ttf
    application/font-woff
    application/vnd.ms-fontobject
    application/vnd.ms-opentype

@aFuzzyBear
Copy link

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.
Assets being generated via pages or components being saved to somewhere in Astro, the /assets/ directory would be a big advantage for us to have, as it would allow us to place our generated assets in a location known to Astro in both SSR and Static build contexts,
For generating files such as images, that are being transformed at run-time, or for downloading font files and storing them somewhere, we really need to have a solid and reliable api to assist us in this endeavour.
I would of liked to see this RFC utterly dismiss the notion of Astro specific components that should be better explored in userland being provided by Astro. I would never wish to come up against this particular fight but I will stand strong on it, a vibrant community is one that has the power to express themselves freely, that includes being able to build components safely and securely within the boundaries set by Astro.
Give us the powers to do what we need to work safely and securely within Astro is the only thing I request.
What maybe Vites way of doing things, isnt necessary right, and its not one we should completely follow, especially with repects to Importing assets of any type inside Astro.
We really need is an Assets folder, if you need to implement base line objects to handle these assets then let them only return the data needed.
We really need is transformation and build hooks so we can influence the build process of Astro, what comes before, during, and post build before Astro signs of on it.
We need better API's to be building plugins for our users, better and safer plugins that they can trust and use without thought or consideration.
Users need safetly when they use our products, we want to guarentee a harmonious experince for them aswell. Give us the clay to build, please, and watch the towers rise.

@FredKSchott
Copy link
Member Author

FredKSchott commented Feb 15, 2022

RFC Call: Thanks everyone for the feedback! Some notes to make sure that history is recorded:

  • We reached formal consensus on the idea of using ESM imports as a consistent API for asset references across Astro files, React files, Svelte, Vue, etc.
  • We did not reach consensus on what the thing returned by that API should look like. More work needed to finalize this part, get involved in the #assets channel on Discord if you are interested in participating!

@jonathantneal
Copy link
Contributor

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.

@FredKSchott
Copy link
Member Author

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 assets/ folder and how that could work (see the proposal for more suggestions on this)

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

Successfully merging this pull request may close these issues.

5 participants