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

Feature request: Add option for soft vs hard icon missing error #441

Open
3 tasks done
thedavidhoffman opened this issue Apr 24, 2024 · 5 comments
Open
3 tasks done

Comments

@thedavidhoffman
Copy link

Describe the problem you'd like to see solved or task you'd like to see made easier

When an icon reference is missing, and you try to use that icon in markup, it will crash an Angular component.

I'd like an option to use a soft versus hard error. So that the function faWarnIfIconDefinitionMissing would either do a console.error or throw a new Error.

Is this in relation to an existing part of angular-fontawesome or something new?

It's related to Fort Awesome checking if an icon reference has been added.

What is 1 thing that we can do when building this feature that will guarantee that it is awesome?

It will make it awesome by ensuring the stability of an application's functionality in the event of a missing icon.

Why would other angular-fontawesome users care about this?

Others might care about this because right now a missing icon will crash a component. Others, like myself might prefer a component run without finding an icon, than crash because it can't find it.

On a scale of 1 (sometime in the future) to 10 (absolutely right now), how soon would you recommend we make this feature?

8 - because it should be relatively easy to implement, it's just adding an option for how the existing faWarnIfIconDefinitionMissing function operates.

Feature request checklist

  • This is a single feature (i.e. not a re-write of all of Font Awesome)
  • The title starts with "Feature request: " and is followed by a clear feature name (Ex: Feature request: moar cowbell)
  • I have searched for existing issues and to the best of my knowledge this is not a duplicate
@bleistift-zwei
Copy link
Contributor

If this feature request is accepted, I’d like to point out that #440 also asks for configuration options that determine whether an error is thrown or a message is logged to console. For consistency, we should probably use the same identifiers in both cases, so #440’s options aren’t called 'throwError' and 'warnInConsole' while this issue’s options are 'throw' and 'logConsole'.

@devoto13
Copy link
Collaborator

Are you aware that you can use FaConfig.fallbackIcon to prevent component from throwing when icon is missing?

Rendering component without any icon is not a valid use, so the proposal of changing error to warning does not sound good to me. However, I would be open to add an opt-in warning when fallback icon is rendered or perhaps expose a missingIconDefinition$ observable on the FaIconLibrary to make it more flexible 🤔

I would guess you were hit by this issue when using icon library approach, because with explicit reference approach missing icon should be caught by type-checking.

@thedavidhoffman
Copy link
Author

@devoto13 we use a mix of the icon library as well as strong typing (imports in the typescript and just html markup for the fa-icon tag). I tried the FaConfig.fallbackIcon, but it didn't work. I still got an error thrown by faWarnIfIconDefinitionMissing. So I'm guessing the fallbackIcon doesn't cover the icon library approach.

I don't want to change the error to a warning, I'd like an opt-in config setting that faWarnIfIconDefinitionMissing() will respect where it will just write an error to the console. So the existing behavior of throwing an error will still be applied by default.

@devoto13
Copy link
Collaborator

I understand that you don't want to change the default, but I don't think that rendering an empty component (with a warning) is a good behaviour. Let me think a bit more about this issue. You're right that fallbackIcon does not work, it's indeed only applied if no icon is provided rather than when icon is provided, but is not added to the library.

@devoto13
Copy link
Collaborator

devoto13 commented May 9, 2024

So my thinking is to add FaIconLibrary.onMissingIcon(func: (prefix: IconPrefix, name: IconName) => IconDefinition | null). This allows consumer to log a warning or do whatever other logic they want and/or return an icon definition to be rendered instead of the missing icon.

This should solve both use cases:

  • Not rendering an empty component under any circumstances.
  • Not crashing when icon definition is missing and logging a warning.

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