Skip to content

Commit

Permalink
fix(core): QueryList first, last and changes are properly typed
Browse files Browse the repository at this point in the history
It is possible that a QueryList has no results at all, and thus `first`
or `last` might be `undefined`. The typing now reflects that reality.

BREAKING CHANGE: if you are using TypeScript strict mode, you must now
check for the existence of `first` and `last` before using them.

BREAKING CHANGE: `QueryList.changes` emissions no longer allow access to
anything. Instead, you must respect the new typing of `QueryList<T>`.
  • Loading branch information
PowerKiKi committed May 7, 2024
1 parent 77ae5a3 commit 670f231
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 26 deletions.
6 changes: 3 additions & 3 deletions goldens/public-api/core/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -1404,7 +1404,7 @@ export class QueryList<T> implements Iterable<T> {
// (undocumented)
[Symbol.iterator]: () => Iterator<T>;
constructor(_emitDistinctChangesOnly?: boolean);
get changes(): Observable<any>;
get changes(): Observable<QueryList<T>>;
destroy(): void;
// (undocumented)
readonly dirty = true;
Expand All @@ -1413,11 +1413,11 @@ export class QueryList<T> implements Iterable<T> {
filter(predicate: (value: T, index: number, array: readonly T[]) => unknown): T[];
find(fn: (item: T, index: number, array: T[]) => boolean): T | undefined;
// (undocumented)
readonly first: T;
readonly first: T | undefined;
forEach(fn: (item: T, index: number, array: T[]) => void): void;
get(index: number): T | undefined;
// (undocumented)
readonly last: T;
readonly last: T | undefined;
// (undocumented)
readonly length: number;
map<U>(fn: (item: T, index: number, array: T[]) => U): U[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ describe('insert/remove', () => {

fixture.componentInstance.currentComponent = InjectedComponent;
fixture.componentInstance.projectables = [
fixture.componentInstance.vcRef.createEmbeddedView(fixture.componentInstance.tplRefs.first)
fixture.componentInstance.vcRef.createEmbeddedView(fixture.componentInstance.tplRefs.first!)
.rootNodes,
];

Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/linker/query_list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,13 @@ export class QueryList<T> implements Iterable<T> {
private _changes: EventEmitter<QueryList<T>> | undefined = undefined;

readonly length: number = 0;
readonly first: T = undefined!;
readonly last: T = undefined!;
readonly first: T | undefined = undefined!;
readonly last: T | undefined = undefined!;

/**
* Returns `Observable` of `QueryList` notifying the subscriber of changes.
*/
get changes(): Observable<any> {
get changes(): Observable<QueryList<T>> {
return (this._changes ??= new EventEmitter());
}

Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/render3/query_reactive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,10 @@ export function bindQueryToSignal(target: Signal<unknown>, queryIndex: number):
node._queryList.onDirty(() => node._dirtyCounter.update((v) => v + 1));
}

function refreshSignalQuery<V>(node: QuerySignalNode<V>, firstOnly: boolean): V | ReadonlyArray<V> {
function refreshSignalQuery<V>(
node: QuerySignalNode<V>,
firstOnly: boolean,
): V | undefined | ReadonlyArray<V> {
const lView = node._lView;
const queryIndex = node._queryIndex;

Expand Down
22 changes: 11 additions & 11 deletions packages/core/test/acceptance/query_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1014,12 +1014,12 @@ describe('query logic', () => {

const contentQList = fixture.componentInstance.contentQueryDir.foos;
expect(contentQList.length).toBe(1);
expect(contentQList.first.nativeElement.getAttribute('id')).toBe('contentAndView');
expect(contentQList.first!.nativeElement.getAttribute('id')).toBe('contentAndView');

const viewQList = fixture.componentInstance.fooBars;
expect(viewQList.length).toBe(2);
expect(viewQList.first.nativeElement.getAttribute('id')).toBe('contentAndView');
expect(viewQList.last.nativeElement.getAttribute('id')).toBe('contentOnly');
expect(viewQList.first!.nativeElement.getAttribute('id')).toBe('contentAndView');
expect(viewQList.last!.nativeElement.getAttribute('id')).toBe('contentOnly');
});
});

Expand Down Expand Up @@ -1380,7 +1380,7 @@ describe('query logic', () => {
const elToQuery = fixture.nativeElement.querySelector('div');

expect(qList.length).toBe(1);
expect(qList.first.nativeElement).toBe(elToQuery);
expect(qList.first!.nativeElement).toBe(elToQuery);
});

it('should query using type predicate and read another directive type', () => {
Expand Down Expand Up @@ -1437,7 +1437,7 @@ describe('query logic', () => {
const elToQuery = fixture.nativeElement.querySelector('div');

expect(qList.length).toBe(1);
expect(qList.first.nativeElement).toBe(elToQuery);
expect(qList.first!.nativeElement).toBe(elToQuery);
});

it('should query for multiple elements and read ElementRef by default', () => {
Expand All @@ -1460,8 +1460,8 @@ describe('query logic', () => {
const elToQuery = fixture.nativeElement.querySelectorAll('div');

expect(qList.length).toBe(2);
expect(qList.first.nativeElement).toBe(elToQuery[0]);
expect(qList.last.nativeElement).toBe(elToQuery[2]);
expect(qList.first!.nativeElement).toBe(elToQuery[0]);
expect(qList.last!.nativeElement).toBe(elToQuery[2]);
});

it('should read ElementRef from an element when explicitly asked for', () => {
Expand All @@ -1483,7 +1483,7 @@ describe('query logic', () => {
const elToQuery = fixture.nativeElement.querySelector('div');

expect(qList.length).toBe(1);
expect(qList.first.nativeElement).toBe(elToQuery);
expect(qList.first!.nativeElement).toBe(elToQuery);
});

it('should query for <ng-container> and read ElementRef with a native element pointing to comment node', () => {
Expand All @@ -1500,7 +1500,7 @@ describe('query logic', () => {

const qList = fixture.componentInstance.query!;
expect(qList.length).toBe(1);
expect(qList.first.nativeElement.nodeType).toBe(Node.COMMENT_NODE);
expect(qList.first!.nativeElement.nodeType).toBe(Node.COMMENT_NODE);
});

it('should query for <ng-container> and read ElementRef without explicit read option', () => {
Expand All @@ -1517,7 +1517,7 @@ describe('query logic', () => {

const qList = fixture.componentInstance.query!;
expect(qList.length).toBe(1);
expect(qList.first.nativeElement.nodeType).toBe(Node.COMMENT_NODE);
expect(qList.first!.nativeElement.nodeType).toBe(Node.COMMENT_NODE);
});

it('should read ViewContainerRef from element nodes when explicitly asked for', () => {
Expand Down Expand Up @@ -1568,7 +1568,7 @@ describe('query logic', () => {

const qList = fixture.componentInstance.query!;
expect(qList.length).toBe(1);
expect(qList.first.nativeElement.nodeType).toBe(Node.COMMENT_NODE);
expect(qList.first!.nativeElement.nodeType).toBe(Node.COMMENT_NODE);
});

it('should read TemplateRef from ng-template by default', () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/core/test/acceptance/view_container_ref_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ describe('ViewContainerRef', () => {
const fixture = TestBed.createComponent(ViewContainerRefApp);
fixture.detectChanges();

const template0 = fixture.componentInstance.vcrComp.templates.first;
const template0 = fixture.componentInstance.vcrComp.templates.first!;
const viewContainerRef = fixture.componentInstance.vcrComp.vcr;
const ref0 = viewContainerRef.createEmbeddedView(template0);

Expand Down Expand Up @@ -717,7 +717,7 @@ describe('ViewContainerRef', () => {
fixture.detectChanges();

const cmpt = fixture.componentInstance;
const viewRef = cmpt.templates.first.createEmbeddedView({});
const viewRef = cmpt.templates.first!.createEmbeddedView({});

// ViewContainerRef is empty and we've got a reference to a view that was not attached
// anywhere
Expand Down
12 changes: 7 additions & 5 deletions packages/core/test/linker/query_integration_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,10 @@ describe('Query API', () => {
const view = createTestCmpAndDetectChanges(MyComp0, template);
const needsTpl: NeedsTpl = view.debugElement.children[0].injector.get(NeedsTpl);

expect(needsTpl.vc.createEmbeddedView(needsTpl.query.first).rootNodes[0]).toHaveText('light');
expect(needsTpl.vc.createEmbeddedView(needsTpl.viewQuery.first).rootNodes[0]).toHaveText(
expect(needsTpl.vc.createEmbeddedView(needsTpl.query.first!).rootNodes[0]).toHaveText(
'light',
);
expect(needsTpl.vc.createEmbeddedView(needsTpl.viewQuery.first!).rootNodes[0]).toHaveText(
'shadow',
);
});
Expand Down Expand Up @@ -592,7 +594,7 @@ describe('Query API', () => {
q.show = true;
view.detectChanges();
expect(q.query.length).toBe(1);
expect(q.query.first.text).toEqual('1');
expect(q.query.first!.text).toEqual('1');
});

it('should not be affected by other changes in the component', () => {
Expand All @@ -601,12 +603,12 @@ describe('Query API', () => {
const q: NeedsViewQueryNestedIf = view.debugElement.children[0].references!['q'];

expect(q.query.length).toEqual(1);
expect(q.query.first.text).toEqual('1');
expect(q.query.first!.text).toEqual('1');

q.show = false;
view.detectChanges();
expect(q.query.length).toEqual(1);
expect(q.query.first.text).toEqual('1');
expect(q.query.first!.text).toEqual('1');
});

it('should maintain directives in pre-order depth-first DOM order after dynamic insertion', () => {
Expand Down

0 comments on commit 670f231

Please sign in to comment.