Relative Path Images

[davidwesst]

This is an incomplete PR that I put together to fix my own relative image issues for my 11ty project, BEFORE I noticed #8 where this is being discussed.

It's a bit of a hack, but I added some conditional logic when setting the src in the plugin, where if the file is not remote and does not exist in the default directory, it checks for is relative to the file it image comes from.

I realize the desired behaviour is a config flag, but I thought I would share this as part of the discussion. Maybe it will help a bit to inspire some people.

Anyway-- I've marked it as draft so people can take a look. I've also added some test data, but they are failing. I'm hoping to take a closer look at it soon to figure out the best way to include tests around the new logic.

Feedback and discussion is appreciated. :)

[solution-loisir]

Very promising! Your solution is using some Eleventy specific code which is inevitable I think. A flag would ensure the default remains neutral. Still, the same logic would apply. I feel that the flag name would have to reflect its Eleventy nature. Maybe something like eleventyResolveToProjectRoot: true and then set it to false to get the other behavior. Thanks for your contributions! Really appreciated! 🎉

[davidwesst]

@solution-loisir Thanks for the feedback yesterday. I took some time to implement the flag as you suggested. I kept the reference to Image.Util.isRemoteUrl as it's also used on line 53. I figure if we want to remove that reference, it could be done as a separate issue. :)

I added a test that builds the site a second time, with only the flag enabled, so the test-eleventy test directory was updated. All the tests appear to be passing, but take a look and see if there is anything more you think it needs.

[davidwesst]

Oh-- and before it can be merged it would need a version bump to 0.8.2 (or something). :)

[solution-loisir]

Great! I had a quick peek and it looks very nice! I'll do a proper review over the weekend when I have time. I'll let you know if there's anything. Thank you so much for this! 😊 Since this is a new feature, version will be bumped to 0.9.0!

[davidwesst]

Woot! Sounds like a plan. Looking forward to the feedback. 😊

[solution-loisir]

The more I think about this, the more I feel that it should be left to users to implement for their own use case instead of forcing opinions. So providing a callback instead of a flag that would give access to env should do the trick. Something along this line:

    if(!Image.Util.isRemoteUrl(normalizedTokenAttributes.src) && resolvePath) {
      src = resolvePath(env);
    }

Or maybe even better just pass env to renderImage. I think this would be the most resilient in the ling run. DX could be dealt with by including examples in the docs (this PR would provide a good example for Eleventy!) Tell me what you think.

[solution-loisir]

Suggested change

	if(!Image.Util.isRemoteUrl(normalizedTokenAttributes.src) && eleventyResolveToProjectRoot == false) {
	if(!Image.Util.isRemoteUrl(normalizedTokenAttributes.src) && env.page.inputPath && eleventyResolveToProjectRoot == false) {

[solution-loisir]

Checking that env.page.inputPath is not undefined. So we don't get an error when used outside Eleventy.

[davidwesst]

That makes a lot of sense, especially combined with the callback you mentioned above. More specifically the resolvePath callback. Thinking of it in my own usecase, I'm not sure how I would implement just a path resolution logic through the renderImage method.

That being said, if you know how it would work, then feel free to share. :)

[solution-loisir]

Yhea you're right, renderImage would not let you implement just a path resolution logic. You would have to implement the full rendering logic as well. It's probably better to go with a resolvePath callback, it's more straightforward. And in any case, renderImage will receive the "proper" path anyway.

[solution-loisir]

Checking for env.page.inputPath with the generic resolvePath callback is likely not relevant anymore. As any code tied to a specific context would be implemented in the callback which receive env.

[davidwesst]

I just pushed the resolvePath change, complete with the env variables. I also included the image path, although I'm unsure if that makes the most sense. Thoughts?

[davidwesst]

Alright-- updated version has been pushed. I added to our conversation about index.js above, but it feels a lot cleaner overall. I didn't include a typecheck on resolvePath, but it appears to be working with whatever logic the author wants to include. Plus, it defaults back to the original behaviour if nothing is passed, which simplifies the code (and removed the dependency on the Image object for that part).

I did, however, include the filepath to the image being managed. Not sure if that was implicit in our conversation or not, but figured I would mention it.

Lastly-- I updated my relative image test to use the new resolvePath parameter, and it works as expected.

Thoughts?

[solution-loisir]

That's great, I'm ready to merge! Thank you so much, it was a pleasure to work this out with you. This feature will be published under v0.9.0! I'll publish in a few days and I let you know. I will likely bring the remote path handling up top so it's above resolvePath and renderImage. That way, remote sources wont have to be handled by users. Cheers! 😄

[davidwesst]

It was great working with you too! Thanks for being so responsive. Now to update my own 11ty project to use the latest version of this code. 😊