chore: Split public and internal interface#440
Conversation
48d4441 to
6da2a40
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #440 +/- ##
==========================================
+ Coverage 91.24% 91.44% +0.20%
==========================================
Files 59 59
Lines 1724 1730 +6
Branches 466 467 +1
==========================================
+ Hits 1573 1582 +9
+ Misses 151 148 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| import { Position } from "../internal/utils/position"; | ||
| import { NonCancelableEventHandler } from "../types/events"; | ||
|
|
||
| export type DataFallbackType = Record<string, unknown>; |
There was a problem hiding this comment.
Is this type now duplicated? (in src/internal/interfaces.ts and here)
There was a problem hiding this comment.
It is, but since the public type lives in the board component - it is not correct to import it from the code that is unrelated to the board. We can create some shared src/types/util.ts - to host the shared interfaces, but this sounds like an overkill in this case.
|
|
||
| export namespace BoardProps { | ||
| export type Item<D = DataFallbackType> = BoardItemDefinition<D>; | ||
| export interface Item<D = DataFallbackType> { |
There was a problem hiding this comment.
This one also introduces a bit of duplication. Would it help to have a "non-internal" common interfaces file?
There was a problem hiding this comment.
This is not quite the duplication, see:
// palette
export interface Item<D = DataFallbackType> {
id: string;
data: D;
definition?: { minRowSpan?: number; minColumnSpan?: number; defaultRowSpan?: number; defaultColumnSpan?: number };
}
// board
export interface Item<D = DataFallbackType> {
id: string;
data: D;
definition?: { minRowSpan?: number; minColumnSpan?: number; defaultRowSpan?: number; defaultColumnSpan?: number };
columnOffset?: { [columns: number]: number };
rowSpan?: number;
columnSpan?: number;
}
We can have some shared types, but I am not sure how to better name it in this case. The Item type actually describes item configuration - relevant for palette and board, but not board-item. I can think of some shared item-definition type maybe, but I see no strong reason to do it now. These item definitions do not actually have to have a common base and can as well evolve independently from one another.
|
|
||
| // our events are not cancelable even though `CustomEvent` may seem to allow this | ||
| export type NonCancelableEventHandler<Detail> = (event: CustomEvent<Detail>) => void; | ||
| export type { NonCancelableEventHandler } from "../../types/events"; |
There was a problem hiding this comment.
Why was this moved? Because this path has "internal" in the name but it is exported publicly? If that is the reason, do we need to keep this re-export? It doesn't seem to have consumers anymore (unless customers import it from here)
There was a problem hiding this comment.
This was an insurance - relevant for the component repo as we do have consumers who import the internal types, unfortunately. However, that is not relevant for the board-components repo - I will remove it.
|
|
||
| export namespace ItemsPaletteProps { | ||
| export type Item<D = DataFallbackType> = BoardItemDefinitionBase<D>; | ||
| export interface Item<D = DataFallbackType> { |
2 participants
Description
Same changes as in components repo: cloudscape-design/components#4703
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.