-
Notifications
You must be signed in to change notification settings - Fork 14
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
Image Directory through CDN: src URL is being converted to a relative URL #863
Comments
Thank you @rrolonNelson, for starting this discussion #861. I have now moved it to an official issue, which we will review with our software engineers. Thank you also to @cbutcosk for your contribution to helping us understand this issue better:
|
As a straw man to capture requirements I wrote the following example test cases const assert = require('node:assert')
const path = require('node:path')
const test = require('node:test')
// "mock" Quire configuration
const config = {
baseURL: 'https://nelson-atkins.org',
cdnURL: 'https://blobcdn.nelson-atkins.org/wpmediablob/',
imageDir: '/assets/images/'
}
/**
* Resolve the full path for an image
* Any valid src URI, including using a 'file:' scheme, is passed through unchanged.
* Relative src paths are qualified with a base URL: either the baseURL or cdnURL.
*
* Nota bene: this code is a "strawman" for handling image path use cases,
* the pattern for integrating this into the current Quire code is TBD...
*/
function resolveImageSrc (src) {
const cdn = src.startsWith('cdn:')
src = cdn ? src.slice(4) : src
let url;
try {
url = new URL(src)
} catch (TypeError) {
const base = cdn
? config.cdnURL
: path.join(config.baseURL, config.imageDir);
url = new URL(src, base)
} finally {
return url
}
}
test('fully qualified URI', () => {
const src = resolveImageSrc('https://blobcdn.nelson-atkins.org/wpmediablob/Starr/figs/5224_c1.png')
assert.equal(src.href, 'https://blobcdn.nelson-atkins.org/wpmediablob/Starr/figs/5224_c1.png')
})
test('path using file: scheme', () => {
const src = resolveImageSrc('file:Starr/figs/5224_c1.png')
assert.equal(src.href, 'file:///Starr/figs/5224_c1.png')
})
test('relative path', () => {
const src = resolveImageSrc('Starr/figs/5224_c1.png')
assert.equal(src.href, 'https://nelson-atkins.org/assets/images/Starr/figs/5224_c1.png')
})
test('path using a pseudo-scheme', () => {
const src = resolveImageSrc('cdn:Starr/figs/5224_c1.png')
assert.equal(src.href, 'https://blobcdn.nelson-atkins.org/wpmediablob/Starr/figs/5224_c1.png')
}) |
@mphstudios Thanks for a peak into a possible solution. That said from my perspective this seems too much complexity for an img tag. Amending the existing but buggy check in the image tag to only prepend the image asset path for relative file paths would have solved this issue for me when I encountered it (which coincidentally also involved a possibility of emitting actual |
I am working on this today. I added the check from |
I made a pull request and am open to feedback. I tried to also get the alt text to be added in but it doesn't seem to want to work #874 |
@rrolonNelson #874 looks good, thank you for submitting the fix; I opened #878 to set the figure image |
Citing @mphstudios's comment in PR #874
This issue has not been resolved for all formats and will remain on backlog for the time being. |
Discussed in #861
Originally posted by rrolonNelson October 10, 2023
We are delivering our images from a CDN with images stored in a blob storage, but have encountered an issue where the src URL is being converted to a relative URL.
config.yaml
imageDir: 'https://blobcdn.nelson-atkins.org/wpmediablob/Starr'
figures.yaml
src: figs/5224_c1.png
desired outcome
https://blobcdn.nelson-atkins.org/wpmediablob/Starr/figs/5224_c1.png
actual outcome
https:/blobcdn.nelson-atkins.org/wpmediablob/Starr/figs/5224_c1.png
which resolves to {URL of current page}/https:/blobcdn.nelson-atkins.org/wpmediablob/Starr/figs/5224_c1.png
In build and preview, we have the same issue when you look at the image src URL but in preview, the images will be visible. (We are using the fix for subdirectories in a build where we had to change the file in the node_modules)
Preview:
Build:
I think the issue comes from using path.join() which will remove any '//'. Based on the code in
image-tag.js
it looks like there is a conditional that checks if the src from figures.yaml starts with an HTTP. To resolve the issue in the light box and inline images we opted to have all the image sources in figures.yaml to have the absolute URL however this seems to break the images on the table of content cards since intable-of-contents/item/image.js
the check for if the src starts with HTTP is not there so it will join the imageDir and src regardless of if it starts with an HTTP and make it a path also.config.yaml
imageDir: 'https://blobcdn.nelson-atkins.org/wpmediablob/Starr'
figures.yaml
src: figs/5224_c1.png
desired outcome
https://blobcdn.nelson-atkins.org/wpmediablob/Starr/figs/5224_c1.png
actual outcome on table of contents
https:/blobcdn.nelson-atkins.org/wpmediablob/Starr/https:/blobcdn.nelson-atkins.org/wpmediablob/Starr/figs/5224_c1.png
which resolves to {URL of current page}https:/blobcdn.nelson-atkins.org/wpmediablob/Starr/https:/blobcdn.nelson-atkins.org/wpmediablob/Starr/figs/5224_c1.png
Build:
Is there a correct way to use images from a blob/cdn or should we endeavor to add the HTTP check to all the places where image src are generated or add a package to join an absolute URL with a path without turning them into relative URLs?
The text was updated successfully, but these errors were encountered: