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

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

Open
dan-cooke opened this issue Oct 3, 2024 · 5 comments
Open

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

dan-cooke opened this issue Oct 3, 2024 · 5 comments

Comments

@dan-cooke
Copy link

dan-cooke commented Oct 3, 2024

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
Copy link
Contributor

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
Copy link
Author

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

Should be an easy fix

@FabulousCodingFox
Copy link
Contributor

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

@FabulousCodingFox
Copy link
Contributor

FabulousCodingFox commented Oct 3, 2024

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
Copy link
Owner

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.

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

3 participants