Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"summary": "#345F92",
},
"type": {
"required": true,
"required": false,
"summary": "string",
},
},
Expand All @@ -23,7 +23,7 @@
"table": {
"category": "outputs",
"type": {
"required": true,
"required": false,
"summary": "(e: string) => void",
},
},
Expand Down Expand Up @@ -56,7 +56,7 @@
"table": {
"category": "outputs",
"type": {
"required": true,
"required": false,
"summary": "(e: boolean) => void",
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"summary": "#345F92",
},
"type": {
"required": true,
"required": false,
"summary": "string",
},
},
Expand All @@ -23,7 +23,7 @@
"table": {
"category": "outputs",
"type": {
"required": true,
"required": false,
"summary": "(e: string) => void",
},
},
Expand Down Expand Up @@ -56,7 +56,7 @@
"table": {
"category": "outputs",
"type": {
"required": true,
"required": false,
"summary": "(e: boolean) => void",
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"summary": "#345F92",
},
"type": {
"required": true,
"required": false,
"summary": "string",
},
},
Expand All @@ -23,7 +23,7 @@
"table": {
"category": "outputs",
"type": {
"required": true,
"required": false,
"summary": "(e: string) => void",
},
},
Expand Down Expand Up @@ -56,7 +56,7 @@
"table": {
"category": "outputs",
"type": {
"required": true,
"required": false,
"summary": "(e: boolean) => void",
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"summary": "#345F92",
},
"type": {
"required": true,
"required": false,
"summary": "string",
},
},
Expand All @@ -23,7 +23,7 @@
"table": {
"category": "outputs",
"type": {
"required": true,
"required": false,
"summary": "(e: string) => void",
},
},
Expand Down Expand Up @@ -56,7 +56,7 @@
"table": {
"category": "outputs",
"type": {
"required": true,
"required": false,
"summary": "(e: boolean) => void",
},
},
Expand Down
12 changes: 11 additions & 1 deletion code/lib/angular-compodoc/src/compodoc-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,17 @@ export interface Property {
decorators?: Decorator[];
/** Omitted by Compodoc for members it cannot type, e.g. `@HostBinding`. */
type?: string;
optional: boolean;
/**
* Whether the member is TS-optional. Compodoc omits it entirely for `@Input()`-decorated
* properties while emitting it for signal inputs and plain class properties (compodoc#863, still
* open at 2.0.0), so it is absent far more often than the old non-optional declaration implied.
*/
optional?: boolean;
/**
* Compodoc's own requiredness flag, which is what Angular actually means by a required input.
* Present for signal inputs and for `@Input({ required })`; absent for a plain `@Input()`.
*/
required?: boolean;
defaultValue?: string;
description?: Html;
rawdescription?: string;
Expand Down
56 changes: 56 additions & 0 deletions code/lib/angular-compodoc/src/extract-arg-types.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,59 @@ describe('extractArgTypesFromData', () => {
expect(() => extract('Status', { enumerations: [{ name: 'Status' }] as never })).not.toThrow();
});
});

describe('required', () => {
/** Extracts a single input declared with the given pair of Compodoc flags. */
const requiredOf = (flags: { optional?: boolean; required?: boolean }) => {
const componentData = {
name: 'StatusComponent',
type: 'component',
inputsClass: [{ name: 'value', type: 'string', ...flags }],
outputsClass: [],
propertiesClass: [],
methodsClass: [],
} as never;

const argTypes = extractArgTypesFromData(componentData, {
compodocJson: jsonWith({} as never),
filterNonInputControls: true,
logger,
unwrapHtml: (html: unknown) => String(html),
});

// `required` has always been written into `table.type`, which the public ArgTypes type
// declares as summary/detail only, so reading it back needs an assertion.
return (argTypes.value.table?.type as { required?: boolean } | undefined)?.required;
};

// One case per shape Compodoc can emit. Which declaration produces which pair is recorded here
// because the pairs are not self-explanatory, and one of them is self-contradictory.
it('is false for a signal input with a default: `input("")`', () => {
expect(requiredOf({ optional: false, required: false })).toBe(false);
});

it('is true for `input.required<T>()`', () => {
expect(requiredOf({ optional: false, required: true })).toBe(true);
});

it('is true for `@Input({ required: true })`', () => {
expect(requiredOf({ optional: false, required: true })).toBe(true);
});

it('is false for `@Input({ required: false })`, which Compodoc reports as required and optional at once', () => {
// Compodoc derives `required` from the presence of the key rather than its value, so this
// declaration contradicts itself. Trusting `required` alone would call it required.
expect(requiredOf({ optional: true, required: true })).toBe(false);
});

it('falls back to `optional` when Compodoc omits `required`', () => {
expect(requiredOf({ optional: true })).toBe(false);
expect(requiredOf({ optional: false })).toBe(true);
});

it('is true for a plain `@Input()`, for which Compodoc emits neither flag (compodoc#863)', () => {
// The remaining upstream gap: with nothing to read, every plain decorator input reads as
// required. Fixing it upstream makes `optional` appear, and this case corrects itself.
expect(requiredOf({})).toBe(true);
});
});
22 changes: 20 additions & 2 deletions code/lib/angular-compodoc/src/extract-arg-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,21 @@ export const isMethod = (methodOrProp: Method | Property): methodOrProp is Metho
return (methodOrProp as Method).args !== undefined;
};

/**
* Whether a member must be bound, from the two flags Compodoc emits about it.
*
* `required` is the flag that matches what Angular means, but it is only trustworthy in one
* direction: Compodoc derives it from the presence of the `required` key in an `@Input({...})`
* argument rather than from its value, so `@Input({ required: false })` reports `required: true`
* alongside `optional: true`. Requiring both to agree keeps that case correct.
*
* `required` is absent altogether for a plain `@Input()`, which falls back to `optional` - and
* Compodoc omits that too (compodoc#863), so those inputs still read as required. That is the
* upstream gap; the moment a fixed Compodoc emits `optional`, this returns the right answer with
* no change here.
*/
const isRequired = (item: Property): boolean => (item.required ?? true) && !item.optional;

export const checkValidComponentOrDirective = (component: Component | Directive) => {
if (!component.name) {
throw new Error(`Invalid component ${JSON.stringify(component)}`);
Expand Down Expand Up @@ -371,7 +386,7 @@ export const extractArgTypesFromData = (
category: section,
type: {
summary: isMethod(item) ? displaySignature(item) : item.type,
required: isMethod(item) ? false : !item.optional,
required: isMethod(item) ? false : isRequired(item as Property),
},
defaultValue: { summary: defaultValue },
},
Expand Down Expand Up @@ -400,7 +415,10 @@ export const extractArgTypesFromData = (
category: 'outputs',
type: {
summary: `(e: ${item.type}) => void`,
required: !item.optional,
// An output is never required to bind, and a real output says so via Compodoc's own
// flag. This one is synthesized, so it has to say so itself rather than inheriting the
// requiredness of the model input it derives from.
required: false,
},
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"summary": false,
},
"type": {
"required": true,
"required": false,
"summary": "boolean",
},
},
Expand All @@ -45,7 +45,7 @@
"summary": 1,
},
"type": {
"required": true,
"required": false,
"summary": "number",
},
},
Expand All @@ -63,7 +63,7 @@ Visible caption next to the control.",
"summary": "",
},
"type": {
"required": true,
"required": false,
"summary": "string",
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"summary": false,
},
"type": {
"required": true,
"required": false,
"summary": "boolean",
},
},
Expand All @@ -45,7 +45,7 @@
"summary": 1,
},
"type": {
"required": true,
"required": false,
"summary": "number",
},
},
Expand All @@ -63,7 +63,7 @@ Visible caption next to the control.",
"summary": "",
},
"type": {
"required": true,
"required": false,
"summary": "string",
},
},
Expand All @@ -82,7 +82,7 @@ Visible caption next to the control.",
"summary": false,
},
"type": {
"required": true,
"required": false,
"summary": "boolean",
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
"table": {
"category": "outputs",
"type": {
"required": true,
"required": false,
"summary": "(e: boolean) => void",
},
},
Expand All @@ -44,7 +44,7 @@ Current text value of the field.",
"summary": "start",
},
"type": {
"required": true,
"required": false,
"summary": "string",
},
},
Expand All @@ -60,7 +60,7 @@ Current text value of the field.",
"table": {
"category": "outputs",
"type": {
"required": true,
"required": false,
"summary": "(e: string) => void",
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
"table": {
"category": "outputs",
"type": {
"required": true,
"required": false,
"summary": "(e: boolean) => void",
},
},
Expand All @@ -44,7 +44,7 @@ Current text value of the field.",
"summary": "start",
},
"type": {
"required": true,
"required": false,
"summary": "string",
},
},
Expand All @@ -60,7 +60,7 @@ Current text value of the field.",
"table": {
"category": "outputs",
"type": {
"required": true,
"required": false,
"summary": "(e: string) => void",
},
},
Expand Down
Loading