inline images #224
inline images #224
Conversation
- Correct concurrency - Return multierr - Use []byte wherever possible - Add semaphore to limit number of workers - Separate timeout for each fetch
| return svg, err | ||
| } | ||
| if resp.err != nil { | ||
| err = multierr.Combine(err, resp.err) |
There was a problem hiding this comment.
Remove multierr and log the errors. Otherwise cmdlog.Logger.Nerrors will always return zero.
There was a problem hiding this comment.
i changed the return svg, nil from ur commit to return svg, err above. so all should be good
There was a problem hiding this comment.
Oh nice catch but I'm saying we get rid of multierr to log all errors as they come in instead of waiting for all images to be fetched before we log errors. Otherwise there's no point in using Nerrors, you could just set a bool in compile() when the imgBundler returns an error and then return an xmain.ExitError with with status 1 if said bool is true at the end of the function.
There was a problem hiding this comment.
hmmm, then this whole thing just doesn't return error at all?
There was a problem hiding this comment.
actually yeah, then there'd be no point in returning an error. which is odd. i think the current way is fine. Nerrors is just checking > 0 anyway, so it's all the same. yes it's the same as setting a bool, but it's cleaner, no extra var
There was a problem hiding this comment.
I don't see why that's odd? It means the CLI is more responsive.
Or actually, yea you ought to return a boolean here then and not even rely on Nerrors, I'm going to remove that. It's not cleaner, it's an unnecessary/nonlocal/magic API.
i.e return an ok bool from this function when the svg is totally good and no images errored and return false when at least one image failed. and then use that bool in compile to return the exit error.
There was a problem hiding this comment.
ok sure. but why return a bool? why not just return the error like before?
There was a problem hiding this comment.
I figure you don't need to, you're going to log that error, you're just going to check if it exists so a simple boolean suffices.
But now that I think about it, just return the xmain.ExitError here and then in compile() return the return error of imgbundler.Inline<whatever> after writing the output. That's perfect.
|
Can we have the bundle flag also inline all the remote images? Or create a separate flag |
|
merging for now, @nhooyr will followup |
is this for Obsidian integration? they don't let SVGs load remote images? |
No I don't know anything about Obsidian. literally nothing. we just talked about it once, creating standalone output svgs with zero linked assets. |
|
bro what, i was asking bernard lol |
|
I had a comment above before him about literally the exact same thing! Didn't even realize he also commented lmfao |
|
Actually idk if I did comment or i was thinking about it and was going to open an issue later lol. yea cause we already have a bundle flag, so we should listen to it for InlineRemote. |
|
yeah for context we're testing obsidian plugins and imgs aren't working rn. we're not sure if obsidian embedded browser has something to prevent remote imgs from loading in svg's or something. in which case, the bundle flag working for svg's will be a blocker for obsidian plugin |
|
ok i'll throw it in my follow up |
Got the method from @berniexie's PR #178 (closes #176 )
Just adapted to use Go to fetch images instead of JS.
Also, reads local images and inlines them in the SVG, since localhost cannot access local files. This closes #146