Feature request: Don’t throw when attempting to add icons to the MockFaIconLibrary

[mwaibel-go]

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

My application uses the icon library approach. Often my components depend on library modules that also make use of FontAwesome. These modules add the icons they require to the FaIconLibrary in their constructor. (This is because I don’t want to maintain a list of required icons for each library and add them to the AppModule manually.)

When testing components that use icons, I use the MockFaIconLibrary to stub out the icons because I don’t really care about what is displayed and don’t want to manually add all the icons to the test. This is a problem when the component also depends on one of my libraries, since then these libraries try to add icons to the MockFaIconLibrary, which throws an error. I would like that error to go away.

I understand that you don’t want users to accidentally import the mock library when they intend to use the real one, but I believe a warning on the console would suffice to notify them. Alternatively, the FontAwesomeTestingModule might have a forRoot method itself that takes a configuration object in which we could specify that adding icons to the icon library is acceptable.

Originally I intended to check whether the FaIconLibrary that’s passed to my angular library is an instance of the MockFaIconLibrary and skip adding the icons in that case. But this isn’t possible because that symbol isn’t exported from '@fortawesome/angular-fontawesome/testing'. So an alternative would be to add the mock library to the API surface.

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

This relates to the existing testing module.

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

Users might not want to have an excessive number of warnings in their console, so I’m warming up to the idea of the module configuration that allows suppressing the warning completely.

Why would other angular-fontawesome users care about this?

I believe this is an issue that others might run into as well, even though no-one has raised this before.

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

3. While it’s no high priority, the required changes seem to be small.

Feature request checklist

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

[devoto13]

I understand that you don’t want users to accidentally import the mock library when they intend to use the real one, but I believe a warning on the console would suffice to notify them. Alternatively, the FontAwesomeTestingModule might have a forRoot method itself that takes a configuration object in which we could specify that adding icons to the icon library is acceptable.

I would expect components from the libraries to use either explicit reference approach or their own icon library instance to not depend on the consumer environment, so your use is a bit usual 🤔 But I guess it's more of a best practice than a strict requirement. So let's change the error to warning by default.

Optionally, we can also add forRoot to allow users to configure the behaviour: whenAddingIcon: 'noop' | 'warning' | 'error' or something similar.

Originally I intended to check whether the FaIconLibrary that’s passed to my angular library is an instance of the MockFaIconLibrary and skip adding the icons in that case. But this isn’t possible because that symbol isn’t exported from '@fortawesome/angular-fontawesome/testing'. So an alternative would be to add the mock library to the API surface.

This is definitely an oversight, it should be a part of the public API.

Feel free to send a PR. Otherwise, I'll do it at some point.

[bleistift-zwei]

I believe this is within my abilities and I can write a PR within the next three weeks.

– mwaibel-go’s alt account.

[bleistift-zwei]

So let's change the error to warning by default.

This would be a breaking change. Are you sure you want to introduce one? I’d suggest keeping the behaviour as it is when the module is unconfigured and only adding the configuration option to disable the error.

[devoto13]

I doubt anybody has had a test which relied on the exception being thrown, but okay let's leave the default as it is today to avoid any surprises.