shepherdjerred / astro-opengraph-images

Generate Open Graph images for your Astro site
https://www.npmjs.com/package/astro-opengraph-images
GNU General Public License v3.0
31 stars 2 forks source link

Remove or make optional the og:image path check #87

Open dan-cooke opened 1 month ago

dan-cooke commented 1 month ago

Hi and thanks for this plugin, this is definetly the best OG plugin for astro as its super flexible - I love the approach of using React and tailwind to create presets.

But I am running into an issue in my monorepo:

Issue

I don't think that this check is neccessary

    if (imageUrl !== pngFile) {
        throw new Error(`The og:image property in ${htmlFile} (${imageUrl}) does not match the generated image (${pngFile}).`);
    }

Rationale

Once the plugin has been properly wired this check should really never fail - the only time it prevents foot guns is when a user is trying out the plugin for the first time.

And at that stage surely the user can just check their app locally to make sure all the og:image tags are correct?

Main reason

This entirely locks out users who run astro in a monorepo. The assumptiuon that

    let imageUrl = new URL(pageDetails.image).pathname;
    // remove leading slash
    imageUrl = imageUrl.slice(1);
    // check that the og:image property matches the sitePath

This code will produce the same url structure on every system is incorrect, for me my built outputs live at ../../dist/apps/myapp/index.html

so this outputDir is not being taken into consideration

Solutions

  1. Remove the check entirely
  2. Make the check optional
  3. Take into consideration the users astro outputDir when performing the path checks

Workaround

I am just using patch-package to remove this check , but I would be happy to contribute a fix upstream once a decision has been made to the best approach

FabulousCodingFox commented 1 month ago

Removing the check isnt a good solution.

I just saw the check uses a fixed dist path relative to the current working directory. This should be changed to the dist directory supplied by Astro

src/hook.ts

// remove leading dist/ from the path
await fs.writeFile(pngFile, resvg.render().asPng());
pngFile = pngFile.replace(path.join(process.cwd(), "dist"), "").replace(/\\/g, "/");
if (pngFile.startsWith("/")) pngFile = pngFile.slice(1);
dan-cooke commented 1 month ago

Yep that’s pretty much the 3rd solution I suggested in the OP

Should be an easy fix

FabulousCodingFox commented 1 month ago

https://docs.astro.build/de/reference/integrations-reference/#dir-option This should be the current output directory

FabulousCodingFox commented 1 month ago

I think line 63 of src/hook.ts just needs to be adjusted. Im not sure though as i cannot test this rn.

- pngFile = pngFile.replace(path.join(process.cwd(), "dist"), "").replace(/\\/g, "/");
+ pngFile = pngFile.replace(fileURLToPath(dir), "").replace(/\\/g, "/");
shepherdjerred commented 1 month ago

I'd like to keep this check in. It's an easy way to us to ensure that the library is working as intended and that users have correctly configured the library.

I originally added the check because I saw users not using the correct URLs, but it also prevents regressions in the plugin itself.

I think your suggestion of considering the output dir would make the most sense. I'd be happy to merge in a PR, and it would be even better if you could also add in a new site into the examples dir that can serve as a test for this functionality.