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

Ability to have images as imports in output #4830

Closed
benmccann opened this issue May 13, 2020 · 11 comments
Closed

Ability to have images as imports in output #4830

benmccann opened this issue May 13, 2020 · 11 comments

Comments

@benmccann
Copy link
Member

Is your feature request related to a problem? Please describe.
I would like images to include a hash so that I can set a long-lived cache expiration and host on a CDN. Dynamic content should not be hashed while static content should be cached basically forever

Describe the solution you'd like
I'd like an option for svelte to import images instead of using the path directly:

import Img from "./statics/image.png";

const myImage = new Image();
myImage = Img;
element.appendChild(myImage);

This will allow webpack to add a hash to the filename.

I can't find the text "img" in the svelte or sapper docs, the RealWorld example doesn't appear to use any static images, and the HN example won't build for me, so I'm having a hard time doing a lot of testing of my own as to the current state of things. E.g. I'm unsure of how static images are handled in rollup today. But based off sveltejs/svelte-loader#114, which is highly voted for, I'm relatively sure that hashing image filenames with webpack is unsupported today.

Describe alternatives you've considered
This was requested in sveltejs/svelte-loader#114. It's possible that svelte-loader could make the generated code compatible with webpack by adding an import to the top of the file and replacing the img urls? But it seems much easier to implement in svelte where you have access to the AST. And doing it in svelte would also make it available to rollup plugins as well vs having to implement it separately for both rollup and webpack

How important is this feature to you?
This is very important because it will allow the files to be cached (https://github.com/sveltejs/sapper/issues/951) and delivered over CDN.

Additional context
Right now Sapper caches everything for 10 minutes including dynamic content which leads to lots of bugs. This would help Sapper have a saner default behavior of disabling caching except when there's a hash in the filename there can be very aggressive caching.

@Conduitry
Copy link
Member

I understand that something very close to this is already possible with the proper bundler plugins. This is outside the purview of the compiler though, which looks at a single .svelte in isolation and returns a compiled version of it, suitable for passing along to a bundler via the appropriate bundler plugin.

@benmccann
Copy link
Member Author

Thanks for the feedback. I was debating where the best place to do it was. The reason I thought it was interesting here is so that we could have access to the AST. I did some more digging and it turns out that the way Vue does it is to have the loader invoke the Vue compiler. They're able to get the AST as a result. I wonder if we should do something similar

@hgl
Copy link

hgl commented May 14, 2020

@Conduitry: If I understand @benmccann correctly, this is the request feature:

<!-- a.svelte -->
<img src="statics/image.png">

gets rendered to

<!-- a.html -->
<img src="statics/image-a218d91.png">

This essentially requires svelte compiler to generate require("./statics/image.png") for the src attribute value when outputting the component js. And similar to vue, ideally it should handle the following attributes:

{
  video: ['src', 'poster'],
  source: 'src',
  img: 'src',
  image: ['xlink:href', 'href'],
  use: ['xlink:href', 'href']
}

Not sure if the core team thinks this is a good idea, but without this, like mentioned in the OP, users have to manually import assets.

@benmccann
Copy link
Member Author

I think @Conduitry understood. His response makes sense. I'm going to close this

@hgl
Copy link

hgl commented May 14, 2020

I think there is still some misunderstanding. As I said, this feature requires the svelte compiler to generate require("./statics/image.png"), which is not "outside the purview of the compiler".

This is not something a bundle loader does. A bundle loader simply calls the compiler to generate the require statements, if I'm not wrong.

@benmccann
Copy link
Member Author

Well import Img from "./statics/image.png"; is not standard javascript, which is what the Svelte compiler outputs. It's something that's webpack-specific. So it makes sense the the webpack loader would be responsible for adding the webpack-specific code

@hgl
Copy link

hgl commented May 14, 2020

If svelte compiler doesn't generate that import statement, there is no way for a webpack loader to know what assets to import. The path simply gets embedded in the generated js code as a regular string.

Ideally, the compiler could provide an option for such generation if standard compliance is a concern.

@pngwn
Copy link
Member

pngwn commented May 14, 2020

This seems the wrong way around, usually, people will want to write their image imports directly as imports because that is statically analysable and visible to the bundler. This also benefits from editor tooling. Writing the image link in the src does not have these benefits and rewriting them at compile time would only provide some of them.

I'd recommend simply importing your images: import img from './my_image_path.png'; and you gain all of the benefits of the above request (plus a few editor specific ones) and it requires no changes to the compiler. You would obviously need a loader or plugin to handle this but that is fine, it is your code.

As a side-note, Svelte doesn't generate require calls for DOM usage.

@hgl
Copy link

hgl commented May 14, 2020

@pngwn Thanks for the input.

because that is statically analysable and visible to the bundler

Maybe I missed something, but if the bundler gets fed what the compiler generates, to the bundler, the generated require statements are statically analyzable?

This also benefits from editor tooling.

I agree with this. But in practice, it's hard to imagine why you would want the editor to analyze assets from require statements in a svelte file? If it's really needed, it's not hard for an editor to statically analyze the aforementioned attributes from a svelte file.

This gain is at best potential imho. OTOH the burden of having to manually write require statements is real. If you agree that every site is supposed to have far future expiry for their static assets, it follows every svelte developer is supposed to manually import static assets, which requires naming every asset and warps syntax.

I personally don't think it's a worthwhile trade feature-wise, though technical-wise, it's debatable whether it's worth the time investment.

@pngwn
Copy link
Member

pngwn commented May 14, 2020

Explicit is better than implicit. When you import directly you as the developer are aware of exactly what is happening as opposed to more behind the scenes magic. Svelte has no opinion on your static assets and people will want to handle them differently. You can absolutely automate the hashing of these assets but that doesn’t have anything to do with svelte.

As for things being statically analysable, that isn’t just of interest to the bundler but to a wide array of tools in your tool chain. Having those imports there in your source unlocks benefits to those pieces of tooling in addition to the bundler.

@hgl
Copy link

hgl commented May 14, 2020

Is being explicit good or not depends on the context imho. Reactivity is implicit, and we can agree it's good in building UI components.

We ultimately disagree with if implicit hashing of static assets is a good idea or not. But I think I can live with the fact that the svelte team think it's not.

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

No branches or pull requests

4 participants