FortAwesome / angular-fontawesome

Official Angular component for Font Awesome 5+
https://fontawesome.com
MIT License
1.49k stars 152 forks source link

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

Closed bleistift-zwei closed 6 months ago

bleistift-zwei commented 6 months ago

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?

bleistift-zwei commented 6 months ago

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.