Skip to content

Commit 1dba707

Browse files
authored
fix(types): properly type check unbound array methods @W-15336947 (#4101)
* fix: use more accurate type for Array#every * feat(types): use better types for unbound array methods * fix: update function signature instead of implementation
1 parent 4cac8a2 commit 1dba707

File tree

17 files changed

+204
-108
lines changed

17 files changed

+204
-108
lines changed

packages/@lwc/engine-core/src/framework/api.ts

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2018, salesforce.com, inc.
2+
* Copyright (c) 2024, Salesforce, Inc.
33
* All rights reserved.
44
* SPDX-License-Identifier: MIT
55
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
@@ -39,6 +39,7 @@ import {
3939
isVScopedSlotFragment,
4040
isVStatic,
4141
Key,
42+
MutableVNodes,
4243
VComment,
4344
VCustomElement,
4445
VElement,
@@ -379,7 +380,7 @@ function i(
379380
iterable: Iterable<any>,
380381
factory: (value: any, index: number, first: boolean, last: boolean) => VNodes | VNode
381382
): VNodes {
382-
const list: VNodes = [];
383+
const list: MutableVNodes = [];
383384
// TODO [#1276]: compiler should give us some sort of indicator when a vnodes collection is dynamic
384385
sc(list);
385386
const vmBeingRendered = getVMBeingRendered()!;
@@ -431,7 +432,8 @@ function i(
431432
if (isArray(vnode)) {
432433
ArrayPush.apply(list, vnode);
433434
} else {
434-
ArrayPush.call(list, vnode);
435+
// `isArray` doesn't narrow this block properly...
436+
ArrayPush.call(list, vnode as VNode | null);
435437
}
436438

437439
if (process.env.NODE_ENV !== 'production') {
@@ -470,20 +472,21 @@ function i(
470472
* [f]lattening
471473
* @param items
472474
*/
473-
function f(items: Readonly<Array<Readonly<Array<VNodes>> | VNodes>>): VNodes {
475+
function f(items: ReadonlyArray<VNodes> | VNodes): VNodes {
474476
if (process.env.NODE_ENV !== 'production') {
475477
assert.isTrue(isArray(items), 'flattening api can only work with arrays.');
476478
}
477479
const len = items.length;
478-
const flattened: VNodes = [];
480+
const flattened: MutableVNodes = [];
479481
// TODO [#1276]: compiler should give us some sort of indicator when a vnodes collection is dynamic
480482
sc(flattened);
481483
for (let j = 0; j < len; j += 1) {
482484
const item = items[j];
483485
if (isArray(item)) {
484486
ArrayPush.apply(flattened, item);
485487
} else {
486-
ArrayPush.call(flattened, item);
488+
// `isArray` doesn't narrow this block properly...
489+
ArrayPush.call(flattened, item as VNode | null);
487490
}
488491
}
489492
return flattened;

packages/@lwc/engine-core/src/framework/base-bridge-element.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2018, salesforce.com, inc.
2+
* Copyright (c) 2024, Salesforce, Inc.
33
* All rights reserved.
44
* SPDX-License-Identifier: MIT
55
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
@@ -68,7 +68,7 @@ function createMethodCaller(methodName: string): (...args: any[]) => any {
6868
const vm = getAssociatedVM(this);
6969
const { callHook, component } = vm;
7070
const fn = (component as any)[methodName];
71-
return callHook(vm.component, fn, ArraySlice.call(arguments));
71+
return callHook(vm.component, fn, ArraySlice.call(arguments as unknown as unknown[]));
7272
};
7373
}
7474

packages/@lwc/engine-core/src/framework/freeze-template.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2018, salesforce.com, inc.
2+
* Copyright (c) 2024, Salesforce, Inc.
33
* All rights reserved.
44
* SPDX-License-Identifier: MIT
55
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
@@ -116,10 +116,10 @@ function warnOnArrayMutation(stylesheets: TemplateStylesheetFactories) {
116116
// we can at least warn when they use the most common mutation methods.
117117
for (const prop of ARRAY_MUTATION_METHODS) {
118118
const originalArrayMethod = getOriginalArrayMethod(prop);
119-
stylesheets[prop] = function arrayMutationWarningWrapper() {
119+
// Assertions used here because TypeScript can't handle mapping over our types
120+
(stylesheets as any)[prop] = function arrayMutationWarningWrapper() {
120121
reportTemplateViolation('stylesheets');
121-
// @ts-expect-error can't properly determine the right `this`
122-
return originalArrayMethod.apply(this, arguments);
122+
return originalArrayMethod.apply(this, arguments as any);
123123
};
124124
}
125125
}

packages/@lwc/engine-core/src/framework/hydration.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ function getValidationPredicate(
174174
// If validationOptOut is an array of strings, attributes specified in the
175175
// array will be "opted out". Attributes not specified in the array will still
176176
// be validated.
177-
if (isArray(optOutStaticProp) && arrayEvery<string>(optOutStaticProp, isString)) {
177+
if (isArray(optOutStaticProp) && arrayEvery(optOutStaticProp, isString)) {
178178
return (attrName: string) => !ArrayIncludes.call(optOutStaticProp, attrName);
179179
}
180180
if (process.env.NODE_ENV !== 'production') {

packages/@lwc/engine-core/src/framework/rendering.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2018, salesforce.com, inc.
2+
* Copyright (c) 2024, Salesforce, Inc.
33
* All rights reserved.
44
* SPDX-License-Identifier: MIT
55
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
@@ -49,6 +49,7 @@ import {
4949
isVScopedSlotFragment,
5050
isVStatic,
5151
Key,
52+
MutableVNodes,
5253
VBaseElement,
5354
VComment,
5455
VCustomElement,
@@ -717,11 +718,11 @@ export function allocateChildren(vnode: VCustomElement, vm: VM) {
717718
* @param children
718719
*/
719720
function flattenFragmentsInChildren(children: VNodes): VNodes {
720-
const flattenedChildren: VNodes = [];
721+
const flattenedChildren: MutableVNodes = [];
721722

722723
// Initialize our stack with the direct children of the custom component and check whether we have a VFragment.
723724
// If no VFragment is found in children, we don't need to traverse anything or mark the children dynamic and can return early.
724-
const nodeStack: VNodes = [];
725+
const nodeStack: MutableVNodes = [];
725726
let fragmentFound = false;
726727
for (let i = children.length - 1; i > -1; i -= 1) {
727728
const child = children[i];
@@ -780,7 +781,7 @@ function createViewModelHook(elm: HTMLElement, vnode: VCustomElement, renderer:
780781
return vm;
781782
}
782783

783-
function allocateInSlot(vm: VM, children: VNodes, owner: VM) {
784+
function allocateInSlot(vm: VM, children: VNodes, owner: VM): void {
784785
const {
785786
cmpSlots: { slotAssignments: oldSlotsMapping },
786787
} = vm;
@@ -807,7 +808,7 @@ function allocateInSlot(vm: VM, children: VNodes, owner: VM) {
807808
// eslint-disable-next-line @typescript-eslint/restrict-plus-operands
808809
const normalizedSlotName = '' + slotName;
809810

810-
const vnodes: VNodes = (cmpSlotsMapping[normalizedSlotName] =
811+
const vnodes: MutableVNodes = (cmpSlotsMapping[normalizedSlotName] =
811812
cmpSlotsMapping[normalizedSlotName] || []);
812813
ArrayPush.call(vnodes, vnode);
813814
}

packages/@lwc/engine-core/src/framework/template.ts

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2018, salesforce.com, inc.
2+
* Copyright (c) 2024, Salesforce, Inc.
33
* All rights reserved.
44
* SPDX-License-Identifier: MIT
55
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
@@ -44,7 +44,7 @@ import {
4444
} from './stylesheet';
4545
import { logOperationEnd, logOperationStart, OperationId } from './profiler';
4646
import { getTemplateOrSwappedTemplate, setActiveVM } from './hot-swaps';
47-
import { VNodes, VStaticPart, VStaticPartElement, VStaticPartText } from './vnodes';
47+
import { MutableVNodes, VNodes, VStaticPart, VStaticPartElement, VStaticPartText } from './vnodes';
4848
import { RendererAPI } from './renderer';
4949
import { getMapFromClassName } from './modules/computed-class-attr';
5050

@@ -370,7 +370,7 @@ export function evaluateTemplate(vm: VM, html: Template): VNodes {
370370
}
371371
const isUpdatingTemplateInception = isUpdatingTemplate;
372372
const vmOfTemplateBeingUpdatedInception = vmBeingRendered;
373-
let vnodes: VNodes = [];
373+
let vnodes: MutableVNodes = [];
374374

375375
runWithBoundaryProtection(
376376
vm,
@@ -446,7 +446,13 @@ export function evaluateTemplate(vm: VM, html: Template): VNodes {
446446
// Set the global flag that template is being updated
447447
isUpdatingTemplate = true;
448448

449-
vnodes = html.call(undefined, api, component, cmpSlots, context.tplCache);
449+
vnodes = html.call(
450+
undefined,
451+
api,
452+
component,
453+
cmpSlots,
454+
context.tplCache
455+
) as MutableVNodes;
450456
const { styleVNodes } = context;
451457
if (!isNull(styleVNodes)) {
452458
ArrayUnshift.apply(vnodes, styleVNodes);

packages/@lwc/engine-core/src/framework/vnodes.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2018, salesforce.com, inc.
2+
* Copyright (c) 2024, Salesforce, Inc.
33
* All rights reserved.
44
* SPDX-License-Identifier: MIT
55
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
@@ -34,7 +34,12 @@ export type VNode =
3434
| VFragment
3535
| VScopedSlotFragment;
3636

37-
export type VNodes = Readonly<Array<VNode | null>>;
37+
export type VNodes = ReadonlyArray<VNode | null>;
38+
/**
39+
* Mutable version of {@link VNodes}. It should only be used inside functions to build an array;
40+
* it should never be used as a parameter or return type.
41+
*/
42+
export type MutableVNodes = Array<VNode | null>;
3843

3944
export interface BaseVParent {
4045
children: VNodes;
@@ -144,7 +149,7 @@ export interface VNodeData {
144149
readonly className?: string;
145150
readonly style?: string;
146151
readonly classMap?: Readonly<Record<string, boolean>>;
147-
readonly styleDecls?: Readonly<Array<[string, string, boolean]>>;
152+
readonly styleDecls?: ReadonlyArray<[string, string, boolean]>;
148153
readonly context?: Readonly<Record<string, Readonly<Record<string, any>>>>;
149154
readonly on?: Readonly<Record<string, (event: Event) => any>>;
150155
readonly svg?: boolean;

packages/@lwc/engine-core/src/framework/wiring/wiring.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2023, salesforce.com, inc.
2+
* Copyright (c) 2024, Salesforce, Inc.
33
* All rights reserved.
44
* SPDX-License-Identifier: MIT
55
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
@@ -246,8 +246,9 @@ export function installWireAdapters(vm: VM) {
246246
vm.debugInfo![WIRE_DEBUG_ENTRY] = create(null);
247247
}
248248

249-
const wiredConnecting = (context.wiredConnecting = []);
250-
const wiredDisconnecting = (context.wiredDisconnecting = []);
249+
const wiredConnecting: VM['context']['wiredConnecting'] = (context.wiredConnecting = []);
250+
const wiredDisconnecting: VM['context']['wiredDisconnecting'] = (context.wiredDisconnecting =
251+
[]);
251252

252253
for (const fieldNameOrMethod in wire) {
253254
const descriptor = wire[fieldNameOrMethod];

packages/@lwc/shared/src/language.ts

Lines changed: 48 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,49 @@ const {
4141
/** Detached {@linkcode Array.isArray}; see {@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray MDN Reference}. */
4242
const { isArray } = Array;
4343

44+
/** The most extensible array type. */
45+
type BaseArray = readonly unknown[];
46+
/** Names of methods that can be used on a readonly array. */
47+
type ArrayPureMethodNames = {
48+
[K in keyof BaseArray]: K extends string
49+
? BaseArray[K] extends (...args: any) => any
50+
? K
51+
: never
52+
: never;
53+
}[keyof BaseArray];
54+
/**
55+
* Unbound array methods, re-typed so that `.call` and `.apply` correctly report type errors.
56+
* @example
57+
* const arr = ['a', 'b', 'c']
58+
* const trim = (str: string) => str.trim()
59+
* const sq = (num: number) => num ** 2
60+
* const unboundForEach = arr.forEach
61+
* unboundForEach.call(arr, trim) // passes - good
62+
* unboundForEach.call(arr, sq) // passes - BAD!
63+
* const fixedForEach = arr.forEach as UnboundArrayPureMethods['forEach']
64+
* fixedForEach.call(arr, trim) // passes - good
65+
* fixedForEach.call(arr, sq) // error - yay!
66+
*/
67+
type UnboundArrayPureMethods = {
68+
[K in ArrayPureMethodNames]: {
69+
call: <T extends BaseArray>(thisArg: T, ...args: Parameters<T[K]>) => ReturnType<T[K]>;
70+
apply: <T extends BaseArray>(thisArg: T, args: Parameters<T[K]>) => ReturnType<T[K]>;
71+
};
72+
};
73+
74+
/** Names of methods that mutate an array (cannot be used on a readonly array). */
75+
type ArrayMutationMethodNames = Exclude<keyof unknown[], keyof BaseArray>;
76+
/**
77+
* Unbound array mutation methods, re-typed so that `.call` and `.apply` correctly report type errors.
78+
* @see {@link UnboundArrayPureMethods} for an example showing why this is needed.
79+
*/
80+
type UnboundArrayMutationMethods = {
81+
[K in ArrayMutationMethodNames]: {
82+
call: <T extends unknown[]>(thisArg: T, ...args: Parameters<T[K]>) => ReturnType<T[K]>;
83+
apply: <T extends unknown[]>(thisArg: T, args: Parameters<T[K]>) => ReturnType<T[K]>;
84+
};
85+
};
86+
4487
// For some reason, JSDoc don't get picked up for multiple renamed destructured constants (even
4588
// though it works fine for one, e.g. isArray), so comments for these are added to the export
4689
// statement, rather than this declaration.
@@ -67,7 +110,7 @@ const {
67110
splice: ArraySplice,
68111
unshift: ArrayUnshift,
69112
forEach, // Weird anomaly!
70-
} = Array.prototype;
113+
}: UnboundArrayPureMethods & UnboundArrayMutationMethods = Array.prototype;
71114

72115
// The type of the return value of Array.prototype.every is `this is T[]`. However, once this
73116
// Array method is pulled out of the prototype, the function is now referencing `this` where
@@ -82,10 +125,10 @@ const {
82125
* @param predicate A function to execute for each element of the array.
83126
* @returns Whether all elements in the array pass the test provided by the predicate.
84127
*/
85-
function arrayEvery<T>(
86-
arr: unknown[],
87-
predicate: (value: any, index: number, array: typeof arr) => value is T
88-
): arr is T[] {
128+
function arrayEvery<S extends T, T = unknown>(
129+
arr: readonly T[],
130+
predicate: (value: any, index: number, array: readonly T[]) => value is S
131+
): arr is readonly S[] {
89132
return ArrayEvery.call(arr, predicate);
90133
}
91134

packages/@lwc/synthetic-shadow/src/faux-shadow/element.ts

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2018, salesforce.com, inc.
2+
* Copyright (c) 2024, Salesforce, Inc.
33
* All rights reserved.
44
* SPDX-License-Identifier: MIT
55
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
@@ -98,7 +98,7 @@ function childrenGetterPatched(this: Element): HTMLCollectionOf<Element> {
9898
? filteredChildNodes
9999
: getAllMatches(owner, filteredChildNodes);
100100
return createStaticHTMLCollection(
101-
ArrayFilter.call(childNodes, (node: Node | Element) => node instanceof Element)
101+
ArrayFilter.call(childNodes, (node) => node instanceof Element) as Element[]
102102
);
103103
}
104104

@@ -235,7 +235,10 @@ if (hasOwnProperty.call(HTMLElement.prototype, 'children')) {
235235

236236
function querySelectorPatched(this: Element /*, selector: string*/): Element | null {
237237
const nodeList = arrayFromCollection(
238-
elementQuerySelectorAll.apply(this, ArraySlice.call(arguments) as [string])
238+
elementQuerySelectorAll.apply(
239+
this,
240+
ArraySlice.call(arguments as unknown as unknown[]) as [string]
241+
)
239242
);
240243
if (isSyntheticShadowHost(this)) {
241244
// element with shadowRoot attached
@@ -340,7 +343,10 @@ defineProperties(Element.prototype, {
340343
querySelectorAll: {
341344
value(this: HTMLBodyElement): NodeListOf<Element> {
342345
const nodeList = arrayFromCollection(
343-
elementQuerySelectorAll.apply(this, ArraySlice.call(arguments) as [string])
346+
elementQuerySelectorAll.apply(
347+
this,
348+
ArraySlice.call(arguments as unknown as unknown[]) as [string]
349+
)
344350
);
345351

346352
// Note: we deviate from native shadow here, but are not fixing
@@ -362,7 +368,7 @@ if (process.env.NODE_ENV !== 'test') {
362368
const elements = arrayFromCollection(
363369
elementGetElementsByClassName.apply(
364370
this,
365-
ArraySlice.call(arguments) as [string]
371+
ArraySlice.call(arguments as unknown as unknown[]) as [string]
366372
)
367373
);
368374

@@ -379,7 +385,10 @@ if (process.env.NODE_ENV !== 'test') {
379385
getElementsByTagName: {
380386
value(this: HTMLBodyElement): HTMLCollectionOf<Element> {
381387
const elements = arrayFromCollection(
382-
elementGetElementsByTagName.apply(this, ArraySlice.call(arguments) as [string])
388+
elementGetElementsByTagName.apply(
389+
this,
390+
ArraySlice.call(arguments as unknown as unknown[]) as [tagName: string]
391+
)
383392
);
384393

385394
// Note: we deviate from native shadow here, but are not fixing
@@ -397,7 +406,10 @@ if (process.env.NODE_ENV !== 'test') {
397406
const elements = arrayFromCollection(
398407
elementGetElementsByTagNameNS.apply(
399408
this,
400-
ArraySlice.call(arguments) as [string, string]
409+
ArraySlice.call(arguments as unknown as unknown[]) as [
410+
namespace: string,
411+
localName: string
412+
]
401413
)
402414
);
403415

0 commit comments

Comments
 (0)