Feature request: Don’t throw when attempting to add icons to the MockFaIconLibrary (#440)

[bleistift-zwei]

This PR implements the feature request from issue 440 to opt out of the exception that is thrown when attempting to add icons to the MockFaIconLibrary. It also adds the MockFaIconLibrary to the public API, as discussed. Additionally, a fix was made to include the existing tests for the testing module in the test runs.

This PR is a WIP, so I have a chance to look at it again tomorrow.

Notes/justifications on the PR

testing/src/testing.module.ts, testing/src/TestingModuleConfig.ts

I used a configuration object rather than a single parameter.
As of now this is probably over-engineered. I still went this route
because in my experience configuration tends to get more complex
over time.

I’ve also decided to split the configuration into two interfaces,
one public and one private. This supports a possible future use case
where the implementation wants to use a new API, but users should
have time to transition and still expect the old configuration interface
to work. Hence there’s the public interface to be provided by users of
the library, a private interface to be used by the library internally,
and a method to translate the public interface to the private one.
The translator function is an implementation detail and not exported.

testing/test/integration/module-test.spec.ts

The added tests are basically the existing tests, copied 4 times.
In each describe block the module is configured differently:

1. Not at all (v0.14.1 behaviour)
2. With an empty object (to test the translator method)
3. {whenAddingIcons: 'logWarning'}
4. {whenAddingIcons: 'throwError'}
5. {whenAddingIcons: 'noop'}

testing/src/icon/mock-icon-library.service.ts

I extracted the error/warning message to a constant, since it is now used
in 4 places, not counting the tests that also assert the message.

Feedback opportunities / issues

All feedback is appreciated. I have already identified these two issues.

I noticed that some of the names got very long, e.g. FontAwesomeTestingModuleConfigInjectionToken. I saw no precedence for shortening them. For instance, FontAwesome is only contract to fa when it occurs in the middle of a symbol name. Note that FontAwesomeTestingModuleConfig is the only long name that’s exported.

It’s debatable whether ‘noop’ should be camel-cased to ‘noOp’ or if a different word should be used altogether.

There is no strict reason to force users of the testing module to configure it only once. That is, instead of implementing the forRoot method we could also go for forChild. I used forRoot for two reasons: First, I don’t see the point of configuring the module multiple times in a test. Second, I believe the forRoot method is more well-known than the forChild, since every routed Angular application needs the RouterModule’s forRoot.

I noticed that the linter touches neither the tests nor the source files in the testing directory: "lintFilePatterns": ["src/**/*.ts", "src/**/*.html"] (source). Is that intentional?

[devoto13]

First of all, thanks for the PR! I have been playing with the implementation and decided to push some changes directly instead of asking you to do them. Could you please check if they make sense to you? If so I'll merge the PR.

---

I noticed that the linter touches neither the tests nor the source files in the testing directory: "lintFilePatterns": ["src//*.ts", "src//*.html"] (source). Is that intentional?

An oversight, same as the tests. We should update the config to include them.

I’ve also decided to split the configuration into two interfaces,
one public and one private. This supports a possible future use case
where the implementation wants to use a new API, but users should
have time to transition and still expect the old configuration interface
to work.

This does not feel necessary at this point and can be introduced if the need arises. It's also not nice that a public type extends a private type. Let's stick to only having a one interface.

[devoto13]

I think if we change from an injection token + object to a class, it solves this issue nicely and is more inline with the FaConfig which is used by the main part of the library.

[bleistift-zwei]

I like your class-based approach better and wish I’d have thought of it. The only thing I would’ve avoided is blindly copying whatever the user provides into the config object at https://github.com/FortAwesome/angular-fontawesome/pull/442/files#diff-6b7d340e6acff58008738e27c885179fe6a9fd9049327879e061c7f00745eecaR25, though that may just be my paranoia speaking.

          useFactory: () => Object.assign(new FaTestingConfig(), config),

Thanks for taking the time to review my PR. Your changes look good.