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

[Fix] Url encoding in images, dont render undefined images #713

Merged
merged 19 commits into from
Nov 2, 2023

Conversation

jonluca
Copy link
Contributor

@jonluca jonluca commented Sep 26, 2023

The library was outputting invalid xml for URLs with query params.

This PR:

  • Adds validation to the XML created
  • Encodes the hrefs
  • Adds safety guards to image creation
  • Tests the new implementation

@vercel
Copy link

vercel bot commented Sep 26, 2023

Someone is attempting to deploy a commit to a Personal Account owned by @iamvishnusankar on Vercel.

@iamvishnusankar first needs to authorize it.

@jonluca
Copy link
Contributor Author

jonluca commented Sep 26, 2023

Fixes #714

@kevinrobert3
Copy link

kevinrobert3 commented Oct 16, 2023

Can this be merged please? Image sitemaps do not work

@kevinrobert3
Copy link

@iamvishnusankar can you merge this

@iamvishnusankar
Copy link
Owner

@jonluca Thanks a lot for this PR. I'm extremely sorry for taking this long to review this. Got busy with few personal projects.

Looks like the build is failing due to incompatibility of node version. Can you please update the action file to set the node versions to ['18', '20'].

node: ['16', '18']

Once you push the changes, I'll merge the PR.

@jonluca
Copy link
Contributor Author

jonluca commented Oct 26, 2023

Done

@jonluca
Copy link
Contributor Author

jonluca commented Oct 27, 2023

Pushed another update updating yarn. We probably shouldn't use yarn set version stable in CI as that'll try and make an actual change if there's a new version

@iamvishnusankar
Copy link
Owner

Pushed another update updating yarn. We probably shouldn't use yarn set version stable in CI as that'll try and make an actual change if there's a new version

@jonluca Yeah, Looks like the command is setting v4 now and it seems not resolving workspace modules. I tried upgrading the project to bun but some tests are not covered by bun:test

@jonluca
Copy link
Contributor Author

jonluca commented Oct 30, 2023

We could probably move to bun for package installs but keep jest for tests

I just pushed an update to use bun to install the packages, but keep jest around for the tests. I think it all passes?

@iamvishnusankar iamvishnusankar changed the title fix: fix url encoding in images, dont render undefined images [Fix] Url encoding in images, dont render undefined images Nov 1, 2023
@iamvishnusankar iamvishnusankar merged commit 0d69597 into iamvishnusankar:master Nov 2, 2023
6 checks passed
@iamvishnusankar
Copy link
Owner

@jonluca Thanks a lot for this PR! I have merged it! 🙏

Looks like there are some issues with the release pipeline. I'll be fixing it this weekend along with much needed fix for app dir routes groups.

@sandrooco
Copy link

Hi @iamvishnusankar, do you already have a rough estimate on when this will be released?
Thank you very much for providing this package!

ariesclark pushed a commit to ariesclark/next-sitemap-x that referenced this pull request Dec 14, 2024
[Fix] Url encoding in images, dont render undefined images
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

Successfully merging this pull request may close these issues.

4 participants