fix: common project should not depend on components project #1258
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1258 +/- ##
===========================================
- Coverage 84.73% 73.22% -11.52%
===========================================
Files 835 836 +1
Lines 17416 17477 +61
Branches 2288 2305 +17
===========================================
- Hits 14758 12797 -1961
- Misses 2624 4639 +2015
- Partials 34 41 +7
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| import { IconType } from '@hypertrace/assets-library'; | ||
| import { APP_TITLE } from '@hypertrace/common'; | ||
| import { NavItemType } from '@hypertrace/components'; | ||
| import { APP_TITLE, NavItemType } from '@hypertrace/common'; |
There was a problem hiding this comment.
circular ref, we're in the common project here
| import { isEqualIgnoreFunctions, throwIfNil } from '../utilities/lang/lang-utils'; | ||
| import { Dictionary } from '../utilities/types/types'; | ||
| import { APP_TITLE, HtRoute } from './ht-route'; | ||
| import { NavItemConfig, NavItemType } from './navigation.config'; |
There was a problem hiding this comment.
IMO the better solution would be to move the method out of nav service into the nav component - that's the only caller and would allow leaving icon size on the config as is, too - but up to you.
There was a problem hiding this comment.
I was actually trying to fix this too as part of nx poc work. I think we should remove the decorateNavItem method from here and move it to a new service -> navigation-list.service.ts. This method is only used when we are using the navigation-list.component.
We can move it to the component too like Aaron is suggesting but that may require some additional work.
| type: NavItemType.Link; | ||
| icon: string; | ||
| iconSize?: IconSize; | ||
| iconSize?: string; |
There was a problem hiding this comment.
I don't think we should remove this type safety. We only support few size options. Moving the method to a new service would solve this.
Description
This PR -> #969 introduced a change where the common project depends on components project. That should not happen. Refactoring code to avoid that unintended dependency.
Testing
Verified with build.
Checklist: