fix: return `null` on invalid icon name exception instead of throwing

stefangalescu / statamic-heroicons

Use and easily customize Heroicons in your Statamic templates

MIT License

2 stars 3 forks source link

fix: return `null` on invalid icon name exception instead of throwing #5

Closed stefangalescu closed 1 year ago

stefangalescu commented 1 year ago

❓ Type of change

[ ] 📖 Content (updates to the content, documentation, API reference or README)
[ ] 🐞 Bug fix (a non-breaking change that fixes an issue)
[ ] 👌 Enhancement (improving an existing functionality, like performance)
[ ] ✨ New feature (a non-breaking change that adds functionality)
[x] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

This PR prevents invalid icon names from throwing an exception and instead make the Antlers tag return nothing.

stefangalescu commented 1 year ago

I can't think of a scenario where I'd want this to throw an exception...

could we set the config option to false as the default?

or maybe just trap the exception in the try-catch and do away with the need for the extra config

(I'm assuming the exception is coming from outside of this package)

@simonhamp Yeah, we can totally do that. The only reason I opted for this is to keep the package working the same even if you don't opt for the config file or setting the option to false. The exception is coming from blade-ui-kit/blade-heroicons. Which option do you suggest we go with?

simonhamp commented 1 year ago

I agree with the principle of it working consistently, but to achieve that while solving the problem at hand has forced the creation of a lot of new code and concepts

Personally, I feel like the default should be to catch the exception and silently carry on - I think this is more in-keeping with the way Antlers tags behave (you may still opt to log the exception, but even that could just become noise for some folks)

That way we can remove all this code and config and just opt for a sensible default that solves the issue

If someone comes asking for the exception to be thrown later on (I suspect we wont get such a request), we can introduce some of this logic again to give them that flexibility

stefangalescu commented 1 year ago

Fair enough. Thanks for explaining your reasoning. I've updated the PR. The only remaining question I have is how do you think we should version this?

simonhamp commented 1 year ago

Well it shouldn't be breaking, so can just be a patch