From a1dcf83105720d9bad191854d5aa285260117436 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Brauer?= Date: Wed, 11 Oct 2023 16:14:30 +0200 Subject: [PATCH] fix: implement setters, descriptors and more Proxy traps (#684) The implementation of #622 only implemented a `get` Proxy trap. This worked fine for the simple use-case of invoking the methods, but failed when users tried to mock functions with Jest or Sinon. It also did not list all functions anymore, when pressing tab-tab in a REPL. This commit implements further Proxy traps which are required for those use-cases and it uses the cache object for mutating operations. Fixes: #683 --- package-lock.json | 124 ++++++++++++++++++++++++++++- package.json | 2 + src/endpoints-to-methods.ts | 38 ++++++++- test/rest-endpoint-methods.test.ts | 100 ++++++++++++++++++++++- 4 files changed, 257 insertions(+), 7 deletions(-) diff --git a/package-lock.json b/package-lock.json index 9c1b7f80c..b9fe19cfa 100644 --- a/package-lock.json +++ b/package-lock.json @@ -17,6 +17,7 @@ "@types/fetch-mock": "^7.3.1", "@types/jest": "^29.0.0", "@types/node": "^18.0.0", + "@types/sinon": "^10.0.19", "esbuild": "^0.19.0", "fetch-mock": "npm:@gr2m/fetch-mock@^9.11.0-pull-request-644.1", "github-openapi-graphql-query": "^4.0.0", @@ -29,6 +30,7 @@ "npm-run-all": "^4.1.5", "prettier": "3.0.3", "semantic-release-plugin-update-version-in-files": "^1.0.0", + "sinon": "^16.1.0", "sort-keys": "^5.0.0", "string-to-jsdoc-comment": "^1.0.0", "ts-jest": "^29.0.0", @@ -1617,13 +1619,40 @@ } }, "node_modules/@sinonjs/fake-timers": { - "version": "10.2.0", + "version": "10.3.0", + "resolved": "https://registry.npmjs.org/@sinonjs/fake-timers/-/fake-timers-10.3.0.tgz", + "integrity": "sha512-V4BG07kuYSUkTCSBHG8G8TNhM+F19jXFWnQtzj+we8DrkpSBCee9Z3Ms8yiGer/dlmhe35/Xdgyo3/0rQKg7YA==", "dev": true, - "license": "BSD-3-Clause", "dependencies": { "@sinonjs/commons": "^3.0.0" } }, + "node_modules/@sinonjs/samsam": { + "version": "8.0.0", + "resolved": "https://registry.npmjs.org/@sinonjs/samsam/-/samsam-8.0.0.tgz", + "integrity": "sha512-Bp8KUVlLp8ibJZrnvq2foVhP0IVX2CIprMJPK0vqGqgrDa0OHVKeZyBykqskkrdxV6yKBPmGasO8LVjAKR3Gew==", + "dev": true, + "dependencies": { + "@sinonjs/commons": "^2.0.0", + "lodash.get": "^4.4.2", + "type-detect": "^4.0.8" + } + }, + "node_modules/@sinonjs/samsam/node_modules/@sinonjs/commons": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/@sinonjs/commons/-/commons-2.0.0.tgz", + "integrity": "sha512-uLa0j859mMrg2slwQYdO/AkrOfmH+X6LTVmNTS9CqexuE2IvVORIkSpJLqePAbEnKJ77aMmCwr1NUZ57120Xcg==", + "dev": true, + "dependencies": { + "type-detect": "4.0.8" + } + }, + "node_modules/@sinonjs/text-encoding": { + "version": "0.7.2", + "resolved": "https://registry.npmjs.org/@sinonjs/text-encoding/-/text-encoding-0.7.2.tgz", + "integrity": "sha512-sXXKG+uL9IrKqViTtao2Ws6dy0znu9sOaP1di/jKGW1M6VssO8vlpXCQcpZ+jisQ1tTFAC5Jo/EOzFbggBagFQ==", + "dev": true + }, "node_modules/@szmarczak/http-timer": { "version": "4.0.6", "dev": true, @@ -1757,6 +1786,21 @@ "@types/node": "*" } }, + "node_modules/@types/sinon": { + "version": "10.0.19", + "resolved": "https://registry.npmjs.org/@types/sinon/-/sinon-10.0.19.tgz", + "integrity": "sha512-MWZNGPSchIdDfb5FL+VFi4zHsHbNOTQEgjqFQk7HazXSXwUU9PAX3z9XBqb3AJGYr9YwrtCtaSMsT3brYsN/jQ==", + "dev": true, + "dependencies": { + "@types/sinonjs__fake-timers": "*" + } + }, + "node_modules/@types/sinonjs__fake-timers": { + "version": "8.1.3", + "resolved": "https://registry.npmjs.org/@types/sinonjs__fake-timers/-/sinonjs__fake-timers-8.1.3.tgz", + "integrity": "sha512-4g+2YyWe0Ve+LBh+WUm1697PD0Kdi6coG1eU0YjQbwx61AZ8XbEpL1zIT6WjuUKrCMCROpEaYQPDjBnDouBVAQ==", + "dev": true + }, "node_modules/@types/stack-utils": { "version": "2.0.1", "dev": true, @@ -2372,6 +2416,15 @@ "node": ">=8" } }, + "node_modules/diff": { + "version": "5.1.0", + "resolved": "https://registry.npmjs.org/diff/-/diff-5.1.0.tgz", + "integrity": "sha512-D+mk+qE8VC/PAUrlAU34N+VfXev0ghe5ywmpqrawphmVZc1bEfn56uo9qpyGp1p4xpzOHkSW4ztBd6L7Xx4ACw==", + "dev": true, + "engines": { + "node": ">=0.3.1" + } + }, "node_modules/diff-sequences": { "version": "29.4.3", "dev": true, @@ -3392,6 +3445,12 @@ "url": "https://github.com/sponsors/ljharb" } }, + "node_modules/isarray": { + "version": "0.0.1", + "resolved": "https://registry.npmjs.org/isarray/-/isarray-0.0.1.tgz", + "integrity": "sha512-D2S+3GLxWH+uhrNEcoh/fnmYeP8E8/zHl644d/jdA0g2uyXvy3sb0qxotE+ne0LtccHknQzWwZEzhak7oJ0COQ==", + "dev": true + }, "node_modules/isexe": { "version": "2.0.0", "dev": true, @@ -4210,6 +4269,12 @@ "node": ">=6" } }, + "node_modules/just-extend": { + "version": "4.2.1", + "resolved": "https://registry.npmjs.org/just-extend/-/just-extend-4.2.1.tgz", + "integrity": "sha512-g3UB796vUFIY90VIv/WX3L2c8CS2MdWUww3CNrYmqza1Fg0DURc2K/O4YrnklBdQarSJ/y8JnJYDGc+1iumQjg==", + "dev": true + }, "node_modules/keyv": { "version": "4.5.2", "dev": true, @@ -4294,6 +4359,12 @@ "dev": true, "license": "MIT" }, + "node_modules/lodash.get": { + "version": "4.4.2", + "resolved": "https://registry.npmjs.org/lodash.get/-/lodash.get-4.4.2.tgz", + "integrity": "sha512-z+Uw/vLuy6gQe8cfaFWD7p0wVv8fJl3mbzXh33RS+0oW2wvUqiRXiQ69gLWSLpgB5/6sU+r6BlQR0MBILadqTQ==", + "dev": true + }, "node_modules/lodash.isequal": { "version": "4.5.0", "dev": true, @@ -4447,6 +4518,37 @@ "dev": true, "license": "MIT" }, + "node_modules/nise": { + "version": "5.1.4", + "resolved": "https://registry.npmjs.org/nise/-/nise-5.1.4.tgz", + "integrity": "sha512-8+Ib8rRJ4L0o3kfmyVCL7gzrohyDe0cMFTBa2d364yIrEGMEoetznKJx899YxjybU6bL9SQkYPSBBs1gyYs8Xg==", + "dev": true, + "dependencies": { + "@sinonjs/commons": "^2.0.0", + "@sinonjs/fake-timers": "^10.0.2", + "@sinonjs/text-encoding": "^0.7.1", + "just-extend": "^4.0.2", + "path-to-regexp": "^1.7.0" + } + }, + "node_modules/nise/node_modules/@sinonjs/commons": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/@sinonjs/commons/-/commons-2.0.0.tgz", + "integrity": "sha512-uLa0j859mMrg2slwQYdO/AkrOfmH+X6LTVmNTS9CqexuE2IvVORIkSpJLqePAbEnKJ77aMmCwr1NUZ57120Xcg==", + "dev": true, + "dependencies": { + "type-detect": "4.0.8" + } + }, + "node_modules/nise/node_modules/path-to-regexp": { + "version": "1.8.0", + "resolved": "https://registry.npmjs.org/path-to-regexp/-/path-to-regexp-1.8.0.tgz", + "integrity": "sha512-n43JRhlUKUAlibEJhPeir1ncUID16QnEjNpwzNdO3Lm4ywrBpBZ5oLD0I6br9evr1Y9JTqwRtAh7JLoOzAQdVA==", + "dev": true, + "dependencies": { + "isarray": "0.0.1" + } + }, "node_modules/node-fetch": { "version": "2.6.11", "dev": true, @@ -5254,6 +5356,24 @@ "url": "https://github.com/sponsors/isaacs" } }, + "node_modules/sinon": { + "version": "16.1.0", + "resolved": "https://registry.npmjs.org/sinon/-/sinon-16.1.0.tgz", + "integrity": "sha512-ZSgzF0vwmoa8pq0GEynqfdnpEDyP1PkYmEChnkjW0Vyh8IDlyFEJ+fkMhCP0il6d5cJjPl2PUsnUSAuP5sttOQ==", + "dev": true, + "dependencies": { + "@sinonjs/commons": "^3.0.0", + "@sinonjs/fake-timers": "^10.3.0", + "@sinonjs/samsam": "^8.0.0", + "diff": "^5.1.0", + "nise": "^5.1.4", + "supports-color": "^7.2.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/sinon" + } + }, "node_modules/sisteransi": { "version": "1.0.5", "dev": true, diff --git a/package.json b/package.json index 7ed481479..a2ff48e8c 100644 --- a/package.json +++ b/package.json @@ -32,6 +32,7 @@ "@types/fetch-mock": "^7.3.1", "@types/jest": "^29.0.0", "@types/node": "^18.0.0", + "@types/sinon": "^10.0.19", "esbuild": "^0.19.0", "fetch-mock": "npm:@gr2m/fetch-mock@^9.11.0-pull-request-644.1", "github-openapi-graphql-query": "^4.0.0", @@ -44,6 +45,7 @@ "npm-run-all": "^4.1.5", "prettier": "3.0.3", "semantic-release-plugin-update-version-in-files": "^1.0.0", + "sinon": "^16.1.0", "sort-keys": "^5.0.0", "string-to-jsdoc-comment": "^1.0.0", "ts-jest": "^29.0.0", diff --git a/src/endpoints-to-methods.ts b/src/endpoints-to-methods.ts index e0c17f472..67105fba4 100644 --- a/src/endpoints-to-methods.ts +++ b/src/endpoints-to-methods.ts @@ -48,14 +48,46 @@ type ProxyTarget = { }; const handler = { + has({ scope }: ProxyTarget, methodName: string) { + return endpointMethodsMap.get(scope).has(methodName); + }, + getOwnPropertyDescriptor(target: ProxyTarget, methodName: string) { + return { + value: this.get(target, methodName), // ensures method is in the cache + configurable: true, + writable: true, + enumerable: true, + }; + }, + defineProperty( + target: ProxyTarget, + methodName: string, + descriptor: PropertyDescriptor, + ) { + Object.defineProperty(target.cache, methodName, descriptor); + return true; + }, + deleteProperty(target: ProxyTarget, methodName: string) { + delete target.cache[methodName]; + return true; + }, + ownKeys({ scope }: ProxyTarget) { + return [...endpointMethodsMap.get(scope).keys()]; + }, + set(target: ProxyTarget, methodName: string, value: any) { + return (target.cache[methodName] = value); + }, get({ octokit, scope, cache }: ProxyTarget, methodName: string) { if (cache[methodName]) { return cache[methodName]; } - const { decorations, endpointDefaults } = endpointMethodsMap - .get(scope) - .get(methodName); + const method = endpointMethodsMap.get(scope).get(methodName); + if (!method) { + return undefined; + } + + const { endpointDefaults, decorations } = method; if (decorations) { cache[methodName] = decorate( diff --git a/test/rest-endpoint-methods.test.ts b/test/rest-endpoint-methods.test.ts index c900e02ab..ae9901c27 100644 --- a/test/rest-endpoint-methods.test.ts +++ b/test/rest-endpoint-methods.test.ts @@ -1,7 +1,9 @@ -import fetchMock from "fetch-mock"; import { Octokit } from "@octokit/core"; +import fetchMock from "fetch-mock"; -import { restEndpointMethods, legacyRestEndpointMethods } from "../src"; +import sinon from "sinon"; +import { legacyRestEndpointMethods, restEndpointMethods } from "../src"; +import { Api } from "../src/types"; describe("REST API endpoint methods", () => { it("README example", async () => { @@ -175,6 +177,100 @@ describe("REST API endpoint methods", () => { return octokit.rest.apps.listInstallations(); }); + describe("mocking", () => { + let octokit: Octokit & Api; + + beforeEach(() => { + const networkMock = fetchMock + .sandbox() + .getOnce( + "https://api.github.com/repos/octokit/plugin-rest-endpoint-methods/issues/1/labels", + [{ name: "mocked from network" }], + ); + + const MyOctokit = Octokit.plugin(restEndpointMethods); + octokit = new MyOctokit({ + request: { + fetch: networkMock, + }, + }); + }); + + afterEach(async () => { + const restoredResult = await octokit.rest.issues.listLabelsOnIssue({ + owner: "octokit", + repo: "plugin-rest-endpoint-methods", + issue_number: 1, + }); + expect(restoredResult.data[0].name).toBe("mocked from network"); + }); + + it("allows mocking with sinon.stub", async () => { + const stub = sinon + .stub(octokit.rest.issues, "listLabelsOnIssue") + .resolves({ data: [{ name: "mocked from sinon" }] } as Awaited< + ReturnType + >); + + const sinonResult = await octokit.rest.issues.listLabelsOnIssue({ + owner: "octokit", + repo: "plugin-rest-endpoint-methods", + issue_number: 1, + }); + expect(sinonResult.data[0].name).toBe("mocked from sinon"); + + stub.restore(); + }); + + it("allows mocking with jest.spyOn", async () => { + jest + .spyOn(octokit.rest.issues, "listLabelsOnIssue") + .mockResolvedValueOnce({ + data: [{ name: "mocked from jest" }], + } as Awaited>); + + const jestResult = await octokit.rest.issues.listLabelsOnIssue({ + owner: "octokit", + repo: "plugin-rest-endpoint-methods", + issue_number: 1, + }); + expect(jestResult.data[0].name).toBe("mocked from jest"); + + jest.restoreAllMocks(); + }); + + it("allows manually replacing a method", async () => { + const oldImplementation = octokit.rest.issues.listLabelsOnIssue; + + octokit.rest.issues.listLabelsOnIssue = (async () => { + return { + data: [{ name: "mocked from custom implementation" }], + } as Awaited>; + }) as unknown as typeof oldImplementation; + + const customResult = await octokit.rest.issues.listLabelsOnIssue({ + owner: "octokit", + repo: "plugin-rest-endpoint-methods", + issue_number: 1, + }); + expect(customResult.data[0].name).toBe( + "mocked from custom implementation", + ); + + delete (octokit.rest.issues as any).listLabelsOnIssue; + octokit.rest.issues.listLabelsOnIssue = oldImplementation; + }); + }); + + it("lists all methods (e.g. with tab-tab in a REPL)", async () => { + const MyOctokit = Octokit.plugin(restEndpointMethods); + const octokit = new MyOctokit(); + + const methods = Object.keys(octokit.rest.issues); + + expect(methods).toContain("listLabelsOnIssue"); + }); + // besides setting `octokit.rest.*`, the plugin exports legacyRestEndpointMethods // which is also setting the same methods directly on `octokit.*` for legacy reasons. // We will deprecate the `octokit.*` methods in future, but for now we just make sure they are set