From a6003b8338b7d9264ca8e3f352d63c3cf5197f81 Mon Sep 17 00:00:00 2001 From: Justin Hopper Date: Wed, 5 Jun 2024 11:44:01 -0500 Subject: [PATCH 1/9] Check configurable when defining property --- lib/callable-instance.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/callable-instance.js b/lib/callable-instance.js index 3e46cabf..8d857a22 100644 --- a/lib/callable-instance.js +++ b/lib/callable-instance.js @@ -30,7 +30,7 @@ export const CallableInstance = for (const p of names) { const descriptor = Object.getOwnPropertyDescriptor(value, p) - if (descriptor) Object.defineProperty(apply, p, descriptor) + if (descriptor && descriptor.configurable) Object.defineProperty(apply, p, descriptor) } return apply From 6433d8523952d9d5f642c4d82efe4ab2edef5e4e Mon Sep 17 00:00:00 2001 From: Justin Hopper Date: Thu, 6 Jun 2024 09:45:14 -0500 Subject: [PATCH 2/9] Fix formatting --- lib/callable-instance.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/callable-instance.js b/lib/callable-instance.js index 8d857a22..83689149 100644 --- a/lib/callable-instance.js +++ b/lib/callable-instance.js @@ -30,7 +30,9 @@ export const CallableInstance = for (const p of names) { const descriptor = Object.getOwnPropertyDescriptor(value, p) - if (descriptor && descriptor.configurable) Object.defineProperty(apply, p, descriptor) + if (descriptor && descriptor.configurable) { + Object.defineProperty(apply, p, descriptor) + } } return apply From 8525da9e7e5a75170f1ead3c95c5a5f2effa9350 Mon Sep 17 00:00:00 2001 From: Justin Hopper Date: Thu, 6 Jun 2024 11:02:07 -0500 Subject: [PATCH 3/9] Add tests for CallableInstance --- package.json | 5 +++- test/callable-instance.js | 61 +++++++++++++++++++++++++++++++++++++++ test/index.js | 1 + 3 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 test/callable-instance.js diff --git a/package.json b/package.json index 9e538ce5..007aee36 100644 --- a/package.json +++ b/package.json @@ -96,7 +96,10 @@ "atLeast": 100, "detail": true, "ignoreCatch": true, - "strict": true + "strict": true, + "ignoreFiles": [ + "test/*.js" + ] }, "xo": { "overrides": [ diff --git a/test/callable-instance.js b/test/callable-instance.js new file mode 100644 index 00000000..eec29243 --- /dev/null +++ b/test/callable-instance.js @@ -0,0 +1,61 @@ +// @ts-nocheck +import assert from 'node:assert/strict' +import test from 'node:test' +import {CallableInstance} from '../lib/callable-instance.js' + +test('callable-instance', async function (t) { + await t.test('can invoke ES6 class', async function () { + class Es6Class extends CallableInstance { + constructor() { + super('copy') + + this.foo = 42 + this.bar = 0 + } + + copy() { + const destination = new Es6Class() + destination.foo = this.foo + destination.bar = this.bar + return destination + } + } + + const instance = new Es6Class() + assert.strictEqual(instance.foo, 42) + + instance.bar = 100 + + // Instance is callable + const copied = instance() + assert.strictEqual(copied.foo, 42) + assert.strictEqual(copied.bar, 100) + }) + + await t.test('can invoke new (ES5 class)', async function () { + function Es5Class() { + const _this = CallableInstance.call(this, ['copy']) + return _this + } + + Es5Class.prototype.foo = 42 + Es5Class.prototype.bar = 0 + + Es5Class.prototype.copy = function () { + const destination = new Es5Class() + destination.foo = this.foo + destination.bar = this.bar + return destination + } + + const instance = new Es5Class() + assert.strictEqual(instance.foo, 42) + + instance.bar = 100 + + // Instance is callable + const copied = instance() + assert.strictEqual(copied.foo, 42) + assert.strictEqual(copied.bar, 100) + }) +}) diff --git a/test/index.js b/test/index.js index 5047cd38..0ee87568 100644 --- a/test/index.js +++ b/test/index.js @@ -1,4 +1,5 @@ /* eslint-disable import/no-unassigned-import */ +import './callable-instance.js' import './core.js' import './data.js' import './freeze.js' From 244a1302410aafff168e916568b71d57dc4fe268 Mon Sep 17 00:00:00 2001 From: Justin Hopper Date: Tue, 11 Jun 2024 11:54:51 -0500 Subject: [PATCH 4/9] Restore type coverage to test files --- package.json | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/package.json b/package.json index 007aee36..9e538ce5 100644 --- a/package.json +++ b/package.json @@ -96,10 +96,7 @@ "atLeast": 100, "detail": true, "ignoreCatch": true, - "strict": true, - "ignoreFiles": [ - "test/*.js" - ] + "strict": true }, "xo": { "overrides": [ From 25ae3e20b8b6db42713a3a55f323af1b6a7fed7f Mon Sep 17 00:00:00 2001 From: Justin Hopper Date: Tue, 11 Jun 2024 11:55:03 -0500 Subject: [PATCH 5/9] Add type coverage to callable instance --- test/callable-instance.js | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/test/callable-instance.js b/test/callable-instance.js index eec29243..e983798c 100644 --- a/test/callable-instance.js +++ b/test/callable-instance.js @@ -1,4 +1,3 @@ -// @ts-nocheck import assert from 'node:assert/strict' import test from 'node:test' import {CallableInstance} from '../lib/callable-instance.js' @@ -9,10 +8,13 @@ test('callable-instance', async function (t) { constructor() { super('copy') + /** @type {number} */ this.foo = 42 + /** @type {number} */ this.bar = 0 } + /** @returns {Es6Class} */ copy() { const destination = new Es6Class() destination.foo = this.foo @@ -27,6 +29,7 @@ test('callable-instance', async function (t) { instance.bar = 100 // Instance is callable + /** @type {Es6Class} */ const copied = instance() assert.strictEqual(copied.foo, 42) assert.strictEqual(copied.bar, 100) @@ -34,13 +37,15 @@ test('callable-instance', async function (t) { await t.test('can invoke new (ES5 class)', async function () { function Es5Class() { - const _this = CallableInstance.call(this, ['copy']) - return _this + /** @type {Function} */ + const callableInstance = CallableInstance + return callableInstance.call(this, ['copy']) } - Es5Class.prototype.foo = 42 - Es5Class.prototype.bar = 0 + Es5Class.prototype.foo = 42 // type-coverage:ignore-line + Es5Class.prototype.bar = 0 // type-coverage:ignore-line + // type-coverage:ignore-next-line Es5Class.prototype.copy = function () { const destination = new Es5Class() destination.foo = this.foo @@ -51,9 +56,12 @@ test('callable-instance', async function (t) { const instance = new Es5Class() assert.strictEqual(instance.foo, 42) + /** @type {number} */ instance.bar = 100 // Instance is callable + /** @type {Es5Class} */ + // @ts-ignore const copied = instance() assert.strictEqual(copied.foo, 42) assert.strictEqual(copied.bar, 100) From de3a4dd6e158ef0b08bdcda95efde7b581dd6232 Mon Sep 17 00:00:00 2001 From: Justin Hopper Date: Tue, 11 Jun 2024 12:08:58 -0500 Subject: [PATCH 6/9] Replace ts-ignore with ts-expect-error --- test/callable-instance.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/callable-instance.js b/test/callable-instance.js index e983798c..56f894da 100644 --- a/test/callable-instance.js +++ b/test/callable-instance.js @@ -53,6 +53,7 @@ test('callable-instance', async function (t) { return destination } + // Es5Class is newable const instance = new Es5Class() assert.strictEqual(instance.foo, 42) @@ -61,7 +62,7 @@ test('callable-instance', async function (t) { // Instance is callable /** @type {Es5Class} */ - // @ts-ignore + // @ts-expect-error Es5Class is also callable const copied = instance() assert.strictEqual(copied.foo, 42) assert.strictEqual(copied.bar, 100) From dbc4e74d2cc827d9f5f251cc57350678f22a87f7 Mon Sep 17 00:00:00 2001 From: Justin Hopper Date: Mon, 17 Jun 2024 10:25:03 -0500 Subject: [PATCH 7/9] Only skip non-configurable fields of target object --- lib/callable-instance.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/callable-instance.js b/lib/callable-instance.js index 83689149..227b1bdf 100644 --- a/lib/callable-instance.js +++ b/lib/callable-instance.js @@ -29,8 +29,14 @@ export const CallableInstance = const names = Object.getOwnPropertyNames(value) for (const p of names) { + const existingDescriptor = Object.getOwnPropertyDescriptor(apply, p) + + if (!existingDescriptor || !existingDescriptor.configurable) { + continue + } + const descriptor = Object.getOwnPropertyDescriptor(value, p) - if (descriptor && descriptor.configurable) { + if (descriptor) { Object.defineProperty(apply, p, descriptor) } } From 26244e625ff3874d80743a6ff9320e0300c689ce Mon Sep 17 00:00:00 2001 From: Justin Hopper Date: Tue, 18 Jun 2024 10:11:40 -0500 Subject: [PATCH 8/9] Remove configurable check and skip property cloning --- lib/callable-instance.js | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/lib/callable-instance.js b/lib/callable-instance.js index 227b1bdf..5f20c5e5 100644 --- a/lib/callable-instance.js +++ b/lib/callable-instance.js @@ -26,20 +26,17 @@ export const CallableInstance = Object.setPrototypeOf(apply, proto) - const names = Object.getOwnPropertyNames(value) - - for (const p of names) { - const existingDescriptor = Object.getOwnPropertyDescriptor(apply, p) - - if (!existingDescriptor || !existingDescriptor.configurable) { - continue - } - - const descriptor = Object.getOwnPropertyDescriptor(value, p) - if (descriptor) { - Object.defineProperty(apply, p, descriptor) - } - } + // Not needed for us in `unified`: we only call this on the `copy` + // function, + // and we don't need to add its fields (`length`, `name`) + // over. + // See also: GH-246. + // const names = Object.getOwnPropertyNames(value) + // + // for (const p of names) { + // const descriptor = Object.getOwnPropertyDescriptor(value, p) + // if (descriptor) Object.defineProperty(apply, p, descriptor) + // } return apply } From c725d6ccf256e4db292106185b820cb00e78060e Mon Sep 17 00:00:00 2001 From: Justin Hopper Date: Tue, 18 Jun 2024 10:11:53 -0500 Subject: [PATCH 9/9] Remove unneeded test for CallableInstance --- test/callable-instance.js | 70 --------------------------------------- test/index.js | 1 - 2 files changed, 71 deletions(-) delete mode 100644 test/callable-instance.js diff --git a/test/callable-instance.js b/test/callable-instance.js deleted file mode 100644 index 56f894da..00000000 --- a/test/callable-instance.js +++ /dev/null @@ -1,70 +0,0 @@ -import assert from 'node:assert/strict' -import test from 'node:test' -import {CallableInstance} from '../lib/callable-instance.js' - -test('callable-instance', async function (t) { - await t.test('can invoke ES6 class', async function () { - class Es6Class extends CallableInstance { - constructor() { - super('copy') - - /** @type {number} */ - this.foo = 42 - /** @type {number} */ - this.bar = 0 - } - - /** @returns {Es6Class} */ - copy() { - const destination = new Es6Class() - destination.foo = this.foo - destination.bar = this.bar - return destination - } - } - - const instance = new Es6Class() - assert.strictEqual(instance.foo, 42) - - instance.bar = 100 - - // Instance is callable - /** @type {Es6Class} */ - const copied = instance() - assert.strictEqual(copied.foo, 42) - assert.strictEqual(copied.bar, 100) - }) - - await t.test('can invoke new (ES5 class)', async function () { - function Es5Class() { - /** @type {Function} */ - const callableInstance = CallableInstance - return callableInstance.call(this, ['copy']) - } - - Es5Class.prototype.foo = 42 // type-coverage:ignore-line - Es5Class.prototype.bar = 0 // type-coverage:ignore-line - - // type-coverage:ignore-next-line - Es5Class.prototype.copy = function () { - const destination = new Es5Class() - destination.foo = this.foo - destination.bar = this.bar - return destination - } - - // Es5Class is newable - const instance = new Es5Class() - assert.strictEqual(instance.foo, 42) - - /** @type {number} */ - instance.bar = 100 - - // Instance is callable - /** @type {Es5Class} */ - // @ts-expect-error Es5Class is also callable - const copied = instance() - assert.strictEqual(copied.foo, 42) - assert.strictEqual(copied.bar, 100) - }) -}) diff --git a/test/index.js b/test/index.js index 0ee87568..5047cd38 100644 --- a/test/index.js +++ b/test/index.js @@ -1,5 +1,4 @@ /* eslint-disable import/no-unassigned-import */ -import './callable-instance.js' import './core.js' import './data.js' import './freeze.js'