nodejs.dev
nodejs.dev copied to clipboard
Reorganise ts-interfaces
Some types are in src/types.ts
, some in dedicated components. We should move them all where they belong and export.src/types.ts
is getting already confusing.
At least for now, imho component specific interfaces (ie props, and maybe queries) are the only interfaces that should in components, whereas interfaces that will be referenced in one or more component-specific ones they belong in src/types.ts
.
My reasoning for this is that it creates a lot more noise when making component changes to conflict with other PRs on src/types.ts
, not to mention the cognitive dissonance from an encapsulation standpoint.
Personal Preference — Over the years, I came to find that encapsulating types with their modules aligns best in the specific case for TypeScript et al, where types merely annotate the code itself.
My preference aside, for our purposes, I am leaning towards leaving component things in component files if everyone else agrees.
It helps if folks want to expose interfaces to use something like:
export const ComponentA = ({body}: ComponentA.Props) => (
<pre>
<code>{body}</code>
</pre>
);
export namespace ComponentA {
export interface Props {
body: string;
}
}
import {ComponentA} from '../components/ComponentA.tsx';
export const LayoutA = () => (
<div>
<ComponentA body={'body'}/>
</div>
);
I see your point @SMotaal . I felt the need while fixing tests. For unit tests, we need to export the props of components. I love the way of accessing props in your example but introducing a namespace just for the sake of syntax seems a bit too much to me. I'm tempted but still, feel a bit ambivalent about that.
My preference aside, for our purposes, I am leaning towards leaving component things in component files if everyone else agrees.
+1
About src/types.ts
... I agree that generic interfaces should be centralised. We just need to find a way to organise these better. When I see all the interfaces used in the project thrown together in a file my OCD rises :]
@LaRuaNa Sorry, I did miss a critical aspect here:
export const ComponentA = ({body}: ComponentA.Props) => (
<pre>
<code>{body}</code>
</pre>
);
export declare namespace ComponentA {
export interface Props {
body: string;
}
}
import {ComponentA} from '../components/ComponentA.tsx';
export const LayoutA = () => (
<div>
<ComponentA body={'body'}/>
</div>
);
So declare
means it is only ambient declarations (ie in .d.ts
). This way it should not add the transpiled and absolutely pointless namespace in case TypeScript does not already drop empty namespace blocks in the transpiled .js
.
Playground — https://www.typescriptlang.org/play/index.html?target=99&jsx=2&ssl=7&ssc=8&pln=7&pc=15#code/KYDwDg9gTgLgBAYwgOwM7wMIQLaWcZGAQTgF44AKAbwCMIATATwF8AuOLXFA4gOgAUoEMKgCUZAHyUAUAEgAPGCjAJchUnoraDFvID0GlXP1KjogNzTpoSLDiaEAGwCGyuMmfZgqMM4TAOHDweEippODgbaHgAS0JgKAAzPwDBYVQ4MIiIuiZ2dCg4gHNLCOZpcqA
@SMotaal Just for my understanding... AFAIK ambient declarations works similar like forward declarations in cpp, right? I don't really get the point. I played around with the code you sent and it doesn't seem like TS-Compiler includes namespaces anyway.
Plus -just for the record- ambient declarations require referencing(?).
We had that discussion in https://github.com/nodejs/nodejs.dev/issues/417 . Consensus is to follow up with the pattern like above. https://github.com/nodejs/nodejs.dev/issues/435#issuecomment-601613636 . There will be a dedicated 'guideline doc' to cover these sort of things in the future.
Can I give this a go if it's still available?