batching fix for chained return objects, docs updates, added test to …

…cover scenario, work towards enabling strict null checks

CHANGELOG.md

@@ -28,7 +28,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
   - Console listener now supports pretty printing options with colors and improved formatting (@thechriskent)
 
 - core:
-  - improved typings on isArray such that TS understands the outcome and properly types arrays
+  - improved typings on utility methods such that TS understands the outcome and properly types results
 
 - queryable:
   - changed constructor to also accept a tuple of [queryable, string] to allow easy rebasing of url while inheriting observers

debug/launch/v3-patrick.ts

@@ -8,6 +8,7 @@ import "@pnp/sp/items";
 import "@pnp/sp/batching";
 import { Web } from "@pnp/sp/webs";
 import { getRandomString } from "@pnp/core";
+import { IItem } from "@pnp/sp/items";
 
 declare var process: { exit(code?: number): void };
 
@@ -55,24 +56,74 @@ export async function Example(settings: ITestingSettings) {
 
         const [batchedSP, execute] = await sp.batched();
 
+        const items = [
+            {
+                Id: 22,
+            },
+            {
+                Id: 23,
+            },
+            {
+                Id: 24,
+            },
+            {
+                Id: 25,
+            }];;
+
+        let res: IItem[] = [];
+
+        for (let i = 0; i < items.length; i++) {
+
+            // you need to use .then syntax here as otherwise the application will stop and await the result
+            batchedSP.web.lists
+                .getByTitle("Generic")
+                .items
+                .getById(items[i].Id)
+                .update({ Title: `${getRandomString(5)}` })
+                .then(r => res.push(r.item));
+        }
+
+        await execute();
+
+        const item = await res[0].select("Id, Title")<{ Id: number, Title: string }>();
+        console.log(`HERE: ${JSON.stringify(item, null, 2)}`);
+
+        const [batchedSP2, execute2] = await sp.batched();
+
+        for (let i = 0; i < items.length; i++) {
+
+            // you need to use .then syntax here as otherwise the application will stop and await the result
+            batchedSP2.web.lists
+                .getByTitle("Generic")
+                .items
+                .add({ Title: `${getRandomString(5)}` })
+                .then(r => res.push(r.item));
+        }
+
+        await execute2();
+
+        const item2 = await res[0].select("Id, Title")<{ Id: number, Title: string }>();
+        console.log(`HERE: ${JSON.stringify(item2, null, 2)}`);
+
+
+
+
         // const list = batchedSP.web.lists.getByTitle("Generic");
 
         // for (let i = 0; i < 3; i++) {
 
         // const items = list.items;
 
-        const y = batchedSP.web.lists.getByTitle("Generic").items.add({
-
-            Title: getRandomString(5),
+        // batchedSP.web.lists.getByTitle("Generic").items.getById(23).update({
 
-        }).then(async r => {
+        //     Title: getRandomString(5),
+
+        // }).then(async r => {
 
-            const y = await r.item.update({
-                Title: "maybe",
-            });
+        //     const y = await r.item();
 
-            console.log(y);
-        });
+        //     console.log(y);
+        // });
 
         // items.add({
         //     Title: getRandomString(5),
@@ -85,13 +136,6 @@ export async function Example(settings: ITestingSettings) {
         // });
         // }
 
-        await execute();
-
-
-
-
-
-
 
     } catch (e) {
 

docs/core/behaviors.md

@@ -119,6 +119,8 @@ Similar to AssignFrom, this method creates a copy of all the observers on the so
 - "replace" will first clear each source moment's registered observers then apply each in source-order via the `on` operation.
 - "append" will apply each source moment's registered observers in source-order via the `on` operation
 
+> By design CopyFrom does NOT include moments defined by symbol keys.
+
 ```TypeScript
 import { spfi, SPBrowser } from "@pnp/sp";
 import { CopyFrom } from "@pnp/core";
@@ -133,7 +135,7 @@ const target = spfi().using(MyCustomeBehavior());
 // any previously registered observers in target are maintained as the default behavior is to append
 target.using(CopyFrom(source));
 
-// target will have the observers assigned from source, but no reference to source. Changes to source's registered observers will not affect target.
+// target will have the observers copied from source, but no reference to source. Changes to source's registered observers will not affect target.
 // any previously registered observers in target are removed
 target.using(CopyFrom(source, "replace"));
 
@@ -142,3 +144,30 @@ target.using(CopyFrom(source, "replace"));
 target.using(SomeOtherBehavior());
 target.on.log(console.log);
 ```
+
+As well `CopyFrom` supports a filter parameter if you only want to copy the observers from a subset of moments. This filter is a predicate function taking a single string key and returning true if the observers from that moment should be copied to the target.
+
+```TypeScript
+import { spfi, SPBrowser } from "@pnp/sp";
+import { CopyFrom } from "@pnp/core";
+// some local project file
+import { MyCustomeBehavior } from "./behaviors.ts";
+
+const source = spfi().using(SPBrowser());
+
+const target = spfi().using(MyCustomeBehavior());
+
+// target will have the observers copied from source, but no reference to source. Changes to source's registered observers will not affect target.
+// any previously registered observers in target are maintained as the default behavior is to append
+target.using(CopyFrom(source));
+
+// target will have the observers `auth` and `send` copied from source, but no reference to source. Changes to source's registered observers will not affect target.
+// any previously registered observers in target are removed
+target.using(CopyFrom(source, "replace", (k) => /(auth|send)/i.test(k)));
+
+// you can always apply additional behaviors or register directly on the events
+// with CopyFrom no reference to source is maintained
+target.using(SomeOtherBehavior());
+target.on.log(console.log);
+```
+

package.json

@@ -52,7 +52,7 @@
         "pnp-publish-v3nightly": "npm run clean && npm run package && pnpbuild -n publish-v3nightly",
         "serve": "tsc -p ./debug/serve/tsconfig.json && node ./build/server/debug/serve/plumbing/run.js",
         "start": "npm run serve",
-        "test": "tsc -p ./test/tsconfig.json && mocha --verbose --cleanup --logging",
+        "test": "tsc -p ./test/tsconfig.json && mocha --verbose --logging",
         "test-build": "tsc -p ./test/tsconfig.json"
     },
     "repository": {

packages/core/behaviors/copy-from.ts

@@ -1,19 +1,20 @@
 import { Timeline, TimelinePipe } from "../timeline.js";
-import { objectDefinedNotNull } from "../util.js";
+import { isFunc, objectDefinedNotNull } from "../util.js";
 import { cloneObserverCollection } from "../timeline.js";
 
 /**
  * Behavior that will copy all the observers in the source timeline and apply it to the incoming instance
  *
  * @param source The source instance from which we will copy the observers
  * @param behavior replace = observers are cleared before adding, append preserves any observers already present
+ * @param filter If provided filters the moments from which the observers are copied. It should return true for each moment to include.
  * @returns The mutated this
  */
-export function CopyFrom(source: Timeline<any>, behavior: "replace" | "append" = "append"): TimelinePipe {
+export function CopyFrom(source: Timeline<any>, behavior: "replace" | "append" = "append", filter?: (moment: string) => boolean): TimelinePipe {
 
     return (instance: Timeline<any>) => {
 
-        return Reflect.apply(copyObservers, instance, [source, behavior]);
+        return Reflect.apply(copyObservers, instance, [source, behavior, filter]);
     };
 }
 
@@ -25,15 +26,19 @@ export function CopyFrom(source: Timeline<any>, behavior: "replace" | "append" =
  * @param behavior replace = observers are cleared before adding, append preserves any observers already present
  * @returns The mutated this
  */
-function copyObservers(this: Timeline<any>, source: Timeline<any>, behavior: "replace" | "append"): Timeline<any> {
+function copyObservers(this: Timeline<any>, source: Timeline<any>, behavior: "replace" | "append", filter?: (moment: string) => boolean): Timeline<any> {
 
     if (!objectDefinedNotNull(source) || !objectDefinedNotNull(source.observers)) {
         return this;
     }
 
+    if (!isFunc(filter)) {
+        filter = () => true;
+    }
+
     const clonedSource = cloneObserverCollection(source.observers);
 
-    const keys = Object.keys(clonedSource);
+    const keys = Object.keys(clonedSource).filter(filter);
 
     for (let i = 0; i < keys.length; i++) {
 

packages/core/extendable.ts

@@ -38,7 +38,9 @@ export function extendable() {
 
                     const extensions = factoryExtensions.get(Reflect.get(proto, ObjExtensionsSym));
 
-                    r = extend(r, extensions);
+                    if (extensions) {
+                        r = extend(r, extensions);
+                    }
                 }
 
                 const proxied = new Proxy(r, {
@@ -99,20 +101,23 @@ export function extendFactory<T extends (...args: any[]) => any>(factory: T, ext
     // factoryExtensions
     const proto = Reflect.getPrototypeOf(factory);
 
-    if (!Reflect.has(proto, ObjExtensionsSym)) {
+    if (proto) {
+        if (!Reflect.has(proto, ObjExtensionsSym)) {
 
-        Reflect.defineProperty(proto, ObjExtensionsSym, {
-            value: getGUID(),
-        });
-    }
+            Reflect.defineProperty(proto, ObjExtensionsSym, {
+                value: getGUID(),
+            });
+        }
 
-    const key = proto[ObjExtensionsSym];
+        const key = proto[ObjExtensionsSym];
 
-    if (!factoryExtensions.has(key)) {
-        factoryExtensions.set(key, []);
-    }
+        if (!factoryExtensions.has(key)) {
+            factoryExtensions.set(key, []);
+        }
 
-    extendCol(factoryExtensions.get(key), extensions);
+        // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+        extendCol(factoryExtensions.get(key)!, extensions);
+    }
 }
 
 function extendCol(a: ExtensionType[], e: ExtensionType | ExtensionType[]) {

packages/core/util.ts

@@ -30,11 +30,12 @@ export function dateAdd(date: Date, interval: DateAddInterval, units: number): D
  *
  * @param paths 0 to n path parts to combine
  */
-export function combine(...paths: string[]): string {
+export function combine(...paths: (string | null | undefined)[]): string {
 
     return paths
         .filter(path => !stringIsNullOrEmpty(path))
-        .map(path => path.replace(/^[\\|/]/, "").replace(/[\\|/]$/, ""))
+        // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+        .map(path => path!.replace(/^[\\|/]/, "").replace(/[\\|/]$/, ""))
         .join("/")
         .replace(/\\/g, "/");
 }
@@ -101,15 +102,15 @@ export function isUrlAbsolute(url: string): boolean {
  *
  * @param s The string to test
  */
-export function stringIsNullOrEmpty(s: string): boolean {
-    return s === undefined || s === null || s.length < 1;
+export function stringIsNullOrEmpty(s: string | undefined | null): s is undefined | null | "" {
+    return typeof s === "undefined" || s === null || s.length < 1;
 }
 
 /**
  * Determines if an object is both defined and not null
  * @param obj Object to test
  */
-export function objectDefinedNotNull(obj: any): boolean {
+export function objectDefinedNotNull<T>(obj: T | undefined | null): obj is T {
     return typeof obj !== "undefined" && obj !== null;
 }
 
@@ -128,10 +129,12 @@ export function jsS(o: any): string {
  * @param o Object to check for
  * @param p Name of the property
  */
-export function hOP(o: any, p: string): boolean {
+export function hOP<T extends string>(o: any, p: T): boolean {
     return Object.hasOwnProperty.call(o, p);
 }
 
+
+
 /**
  * Generates a ~unique hash code
  *

packages/graph/batching.ts

@@ -65,7 +65,7 @@ interface IGraphBatchResponse {
     nextLink?: string;
 }
 
-type ParsedGraphResponse = { nextLink: string; responses: Response[] };
+type ParsedGraphResponse = { nextLink?: string; responses: Response[] };
 
 /**
  * The request record defines a tuple that is
@@ -119,7 +119,7 @@ class BatchQueryable extends _GraphQueryable {
             const m = url.match(versRegex);
 
             // if we don't have the match we expect we don't make any changes and hope for the best
-            if (m.length > 0) {
+            if (m && m.length > 0) {
                 // fix up the url, requestBaseUrl, and the _url
                 url = combine(m[0], "$batch");
                 this.requestBaseUrl = url;
@@ -218,16 +218,12 @@ export function createBatch(base: IGraphQueryable, props?: IGraphBatchProps): [T
         // we replace the send function with our batching logic
         instance.on.send.replace(async function (this: Queryable, url: URL, init: RequestInit) {
 
-            let requestTuple: RequestRecord;
-
             const promise = new Promise<Response>((resolve, reject) => {
-                requestTuple = [this, url.toString(), init, resolve, reject];
+                requests.push([this, url.toString(), init, resolve, reject]);
             });
 
             this.log(`[batch:${batchId}] (${(new Date()).getTime()}) Adding request ${init.method} ${url.toString()} to batch.`, 0);
 
-            requests.push(requestTuple);
-
             // we need to ensure we wait to resolve execute until all our batch children have fully completed their request timelines
             completePromises.push(new Promise((resolve) => {
                 this[RequestCompleteSym] = resolve;
@@ -302,7 +298,8 @@ function formatRequests(requests: RequestRecord[], batchId: string): IGraphBatch
 
         let requestFragment: IGraphBatchRequestFragment = {
             id: `${++index}`,
-            method: init.method,
+            // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+            method: init.method!,
             url: makeUrlRelative(url),
         };
 

packages/graph/behaviors/graphbrowser.ts

@@ -20,7 +20,7 @@ export function GraphBrowser(props?: IGraphBrowserProps): TimelinePipe<Queryable
             BrowserFetchWithRetry(),
             DefaultParse());
 
-        if (isUrlAbsolute(props?.baseUrl)) {
+        if (props?.baseUrl) {
 
             // we want to fix up the url first
             instance.on.pre.prepend(async (url, init, result) => {

packages/graph/behaviors/telemetry.ts

@@ -7,10 +7,9 @@ export function Telemetry(): TimelinePipe<Queryable> {
 
         instance.on.pre(async function (this: Queryable, url, init, result) {
 
-            init.headers = { ...init.headers, ["SdkVersion"]: "PnPCoreJS/$$Version$$" };
-
-            // eslint-disable-next-line @typescript-eslint/dot-notation
-            this.log(`Request Tag: ${init.headers["SdkVersion"]}`, 0);
+            init.headers = { ...init.headers, SdkVersion: "PnPCoreJS/$$Version$$" };
+            // eslint-disable-next-line @typescript-eslint/no-non-null-assertion, @typescript-eslint/dot-notation
+            this.log(`Request Tag: ${init.headers!["SdkVersion"]}`, 0);
 
             return [url, init, result];
         });

packages/graph/graphqueryable.ts

@@ -142,7 +142,7 @@ export class _GraphQueryableCollection<GetType = any[]> extends _GraphQueryable<
      */
     public orderBy(orderBy: string, ascending = true): this {
         const o = "$orderby";
-        const query = this.query.has(o) ? this.query.get(o).split(",") : [];
+        const query = this.query.get(o)?.split(",") || [];
         query.push(`${encodeURIComponent(orderBy)} ${ascending ? "asc" : "desc"}`);
         this.query.set(o, query.join(","));
         return this;