Skip to content

Conversation

@silverbackdan
Copy link
Contributor

πŸ”— Linked issue

#466

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

I am rolling through all the issues that are coming up from typescript fixes. Some of the changes are code and variables may be undefined in places. These could cause potential crashes or bugs - and I'm not sure if any of them will cause my specific problem but better to eliminate the possibilities first as I read into the code.

@silverbackdan
Copy link
Contributor Author

Possible breaking changes here: 2c1ab34

No breaking changes intended, but many types that may be historic? Or just when things were being re-worked? Some objects are types as string|URL but had some assumptions of objects in there.

This PR does need verifying, but I'm just doing my best to make sure types match and where properties were missing I've been adding them, if they seem unused and undocumented I'm changing.

By the end the pnpm lint:type will not yield errors. Perhaps worth putting this check into the CI.

This could catch a lot of potential bugs IMHO

@silverbackdan
Copy link
Contributor Author

I have finished going through the files, fixing types and making sure that all the code should be expecting and handling the correct data types throughout. The script lint:type now passes without errors - the only file I skipped was for a Nuxt v2 version.

Many properties didn't seem quite right, I think probably a lot of little bugs are resolved throughout - but there is a chance of breaking changes. As such, with a change like this, if we do not have in-depth automated tests I may recommend a major release version, and possibly deprecating support of Nuxt v2 - especially as Nuxt v4 is now out and I imagine you'd want to start updating your builders.

I updated my module to Nuxt v4 and the type resolution was better to be honest. It wasn't much work. It may be time to encourage people to update from v2 if they use the module... ?? That way before you update we could also remove all v2 related code, where types are not matching.

Please do review at your leisure, happy to adjust and try and remember each part I changed and why. Some updates were just for the benefit of the tsc analysis where it didn't resolve that something clearly would not be undefined.

I'm deploying this forked branch to my CK website now and I'll let you know if it had any impact on the sitemap disappearing, or if any errors are now visibly thrown when the sitemap begins to disappear.

@silverbackdan silverbackdan marked this pull request as ready for review October 1, 2025 13:21
@silverbackdan
Copy link
Contributor Author

silverbackdan commented Oct 1, 2025

I have deployed and realise something I've done in here has meant that the sitemap no longer populates so I'm going to have to fix this first. Possibly good news in a way - the same bug is presenting consistently with no errors showing in the logs or console - yet all your module tests pass

@silverbackdan
Copy link
Contributor Author

FYI my module is here that implements the functionality
https://github.com/components-web-app/cwa-nuxt-module

But to run it all with the API, docker compose is required and the template for it all together is here - now that my template application is affected in a more minimalistic implementation it may be worth linking you
https://github.com/components-web-app/components-web-app

Just in case you wanted or had time to drop in,

@silverbackdan
Copy link
Contributor Author

silverbackdan commented Oct 1, 2025

This one was a simple bug, I did some bad changes to schema files to use https, and then the xsl style file didn't find the xml properly. Schemas now reverted, the sitemap is showing, will see if it disappears. Just about to update the CWA

@harlan-zw
Copy link
Collaborator

harlan-zw commented Oct 5, 2025

Thanks for the hard work on this. This is mostly good to go, there are just a few small details I need to dig into and see if there's a better solution. It will definitely be merged next time I'm working on the module.

In the meantime, if you wanted to just send PRs for the issues you discovered and fixed happy to try get them released asap.

@silverbackdan
Copy link
Contributor Author

Thanks - I can't actually remember as I was just going through as a chore - but I remember thinking it was strange that a type was a bit different, or there were sitemap xml output rules for properties that weren't defined in types, or were an object instead of a URL.

I reckon a slower review is a good plan, and I've allowed edits by maintainers so you're welcome to take this PR and do anything you wish. Hopefully it gives you a good base to look through all the type issues to resolve.

I'd be interested to see what you review and change, always happy to learn.

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.

2 participants