fix: broken robots:config normalizing
#234
Conversation
|
@silverbackdan could you please review 🙏 I still need to investigate the double hook call. |
|
@harlan-zw thanks for jumping on this so quickly. The fix and tests all look like a huge improvements. Do you think that I'll check the implementation from this branch in my application using the currently released sitemap module and report back. |
|
This fixed branch and currently released sitemap module is deployed now to https://www.cymrukitchens.com/sitemap_index.xml These sitemaps in this website are the ones that would disappear before so we'll see in an hour or so - but from the work you've done and tests in place this is sure to fix it. |
| ] | ||
| for (const group of groups) { | ||
| if (!group._indexable) { | ||
| if (group._indexable === false) { |
There was a problem hiding this comment.
This alone would have kept my sitemaps populated :) :)
| } | ||
| await nitro.hooks.callHook('robots:config', generateRobotsTxtCtx) | ||
| // Normalize groups after hook to ensure all groups have _indexable property | ||
| generateRobotsTxtCtx.groups = generateRobotsTxtCtx.groups.map(normalizeGroup) |
There was a problem hiding this comment.
It does seem a shame to have to reprocess all the groups still doesn't it? It's just on the off change the hook call is calling a function which is designed to extend the config. Is it worth a different hook name, or adding a specific robots:config-extend hook just in the module? .. but I suppose if the groups are passed in any hook, they have the possibility of being altered.
There was a problem hiding this comment.
So ideally we only normalise once but yeh the issue is we're giving the user normalised data that they can then modify (and un-normalise) we can't switch that around without a breaking change.
There's likely a optimisation around it but it adds complexity so this is the safest for now most likely but will review that before I merge
|
Still fixed on my website, so I'm going to say the patch is a success 🥳 |
|
The other issue I reported in sitemap where the titles and content of the sitemap didn't update when linking between pages in production seems to have resolve it seems for now too.. |
|
So the thinking is to deprecate this runtime hook and introduce two new hooks for more fine-grain control over the robots with more preditable behavior. I'll attempt this in another PR with less urgency. |
|
That sounds good to me. We could possibly even add them in as additional hooks while this one is deprecated so not to cause breaking changes for a while. I did have a thought that perhaps I was adding the same groups in again into the hook, but the logs I made didn't seem to suggest it at the time. I'm not entirely sure of how lots of the system work still, if this data is simply saved one in the server-side instance - or what specific event was triggering the sitemap to disappear after some time. But this fix does work, and these can also be questions worth considering in a less urgent way as the functionality of the applications I've implemented this on no longer have bugs, so thank you very much for your work over the weekend on this! |
|
Ps - if I get a chance do you want me to go through like I did on sitemap to resolve all typecheck issues? |
|
The sitemap module is one of my last outstanding modules where the types aren't checked in the CI, so robots should be good. I've started working on #235 for the new hooks and any other changes to your initial issue. If you're interested in helping, I gave you write access; you could work on the same branch locally. All good if not though. I am very open to reworking any code I've done in that PR I was just playing around to see what would be useful. |
|
Thanks very much, I'll write some comments there. |
🔗 Linked issue
#233
❓ Type of change
📚 Description
Two related bugs affecting robots.txt configuration:
.includes()bug innormalizeGroup()- Used.includes()with a callback instead of.some(), causing_indexableto always betrueeven whendisallow: ['/']should make itfalseMissing normalization after
robots:confighook - Groups added via the hook weren't normalized, leaving them without the_indexableproperty. This caused all URLs to be incorrectly marked as non-indexable,breaking sitemap generation for users relying on this hook.