From 8af1f033d79ab14968f0862cedfa41bb3aa9dd64 Mon Sep 17 00:00:00 2001 From: Julien Deniau Date: Fri, 20 Nov 2020 08:49:34 +0100 Subject: [PATCH 1/5] throw Error when passing a instance of Record as a Record factory default values Fixes https://github.com/immutable-js-oss/immutable-js/issues/127 --- __tests__/Record.ts | 10 ++++++++++ src/Record.js | 6 ++++++ 2 files changed, 16 insertions(+) diff --git a/__tests__/Record.ts b/__tests__/Record.ts index 76ad89da86..e68242ba9b 100644 --- a/__tests__/Record.ts +++ b/__tests__/Record.ts @@ -261,4 +261,14 @@ describe('Record', () => { expect(factoryA().equals(factoryA())).toBe(true); expect(factoryA().equals(factoryB())).toBe(true); }); + + it('does not accept a Record as constructor', () => { + const Foo = Record({ foo: 'bar' }); + const fooInstance = Foo(); + expect(() => { + Record(fooInstance); + }).toThrow( + 'Can not use an immutable Record instance as default values for another Record factory. You may want to use a real javascript object instead.' + ); + }); }); diff --git a/src/Record.js b/src/Record.js index 2731267a49..cf6d8a1d0e 100644 --- a/src/Record.js +++ b/src/Record.js @@ -33,6 +33,12 @@ export class Record { constructor(defaultValues, name) { let hasInitialized; + if (isRecord(defaultValues)) { + throw new Error( + 'Can not use an immutable Record instance as default values for another Record factory. You may want to use a real javascript object instead.' + ); + } + const RecordType = function Record(values) { if (values instanceof RecordType) { return values; From bcbe84c330a4c21d71e5b73edd0fe3490ce76c17 Mon Sep 17 00:00:00 2001 From: Julien Deniau Date: Fri, 20 Nov 2020 14:38:44 +0100 Subject: [PATCH 2/5] allow Record as a Record defaultValues --- __tests__/Record.ts | 9 ++++----- src/Record.js | 10 +++------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/__tests__/Record.ts b/__tests__/Record.ts index e68242ba9b..25d9c47562 100644 --- a/__tests__/Record.ts +++ b/__tests__/Record.ts @@ -265,10 +265,9 @@ describe('Record', () => { it('does not accept a Record as constructor', () => { const Foo = Record({ foo: 'bar' }); const fooInstance = Foo(); - expect(() => { - Record(fooInstance); - }).toThrow( - 'Can not use an immutable Record instance as default values for another Record factory. You may want to use a real javascript object instead.' - ); + const Bar = Record(fooInstance); + const barInstance = Bar(); + + expect(barInstance.toJS()).toEqual(fooInstance.toJS()); }); }); diff --git a/src/Record.js b/src/Record.js index cf6d8a1d0e..d44a0b0abf 100644 --- a/src/Record.js +++ b/src/Record.js @@ -33,12 +33,6 @@ export class Record { constructor(defaultValues, name) { let hasInitialized; - if (isRecord(defaultValues)) { - throw new Error( - 'Can not use an immutable Record instance as default values for another Record factory. You may want to use a real javascript object instead.' - ); - } - const RecordType = function Record(values) { if (values instanceof RecordType) { return values; @@ -48,7 +42,9 @@ export class Record { } if (!hasInitialized) { hasInitialized = true; - const keys = Object.keys(defaultValues); + const keys = Object.keys( + isRecord(defaultValues) ? defaultValues.toObject() : defaultValues + ); const indices = (RecordTypePrototype._indices = {}); // Deprecated: left to attempt not to break any external code which // relies on a ._name property existing on record instances. From e0b08d00016ab5d55b596bf8e34b3f2827dd00f6 Mon Sep 17 00:00:00 2001 From: Julien Deniau Date: Fri, 20 Nov 2020 15:47:15 +0100 Subject: [PATCH 3/5] accept Record, throw on non-objects or collection --- __tests__/Record.ts | 16 +++++++++++++- __tests__/__snapshots__/Record.ts.snap | 5 +++++ src/Record.js | 29 ++++++++++++++++++++++---- 3 files changed, 45 insertions(+), 5 deletions(-) create mode 100644 __tests__/__snapshots__/Record.ts.snap diff --git a/__tests__/Record.ts b/__tests__/Record.ts index 25d9c47562..1f528c9057 100644 --- a/__tests__/Record.ts +++ b/__tests__/Record.ts @@ -7,7 +7,7 @@ /// -import { isKeyed, Record, Seq } from '../'; +import { isKeyed, Map, Record, Seq } from '../'; describe('Record', () => { it('defines a constructor', () => { @@ -270,4 +270,18 @@ describe('Record', () => { expect(barInstance.toJS()).toEqual(fooInstance.toJS()); }); + + it('does not accept a non object as constructor', () => { + const defaultValues = null; + expect(() => { + Record(defaultValues); + }).toThrowErrorMatchingSnapshot(); + }); + + it('does not accept an immutable object that is not a Record as constructor', () => { + const defaultValues = Map({ foo: 'bar' }); + expect(() => { + Record(defaultValues); + }).toThrowErrorMatchingSnapshot(); + }); }); diff --git a/__tests__/__snapshots__/Record.ts.snap b/__tests__/__snapshots__/Record.ts.snap new file mode 100644 index 0000000000..401c03fb5e --- /dev/null +++ b/__tests__/__snapshots__/Record.ts.snap @@ -0,0 +1,5 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`Record does not accept a non object as constructor 1`] = `"Can not call \`Record\` with a non-object as default values. You may want to use a javascript object."`; + +exports[`Record does not accept an immutable object that is not a Record as constructor 1`] = `"Can not call \`Record\` an immutable Collection as default values. You mant want to use a plain javascript object instead."`; diff --git a/src/Record.js b/src/Record.js index d44a0b0abf..ade042d977 100644 --- a/src/Record.js +++ b/src/Record.js @@ -28,11 +28,34 @@ import { asImmutable } from './methods/asImmutable'; import invariant from './utils/invariant'; import quoteString from './utils/quoteString'; +import { isImmutable } from './predicates/isImmutable'; + +function convertDefaultValues(defaultValues) { + if (isRecord(defaultValues)) { + return defaultValues.toObject(); + } + + if (isImmutable(defaultValues)) { + throw new Error( + 'Can not call `Record` an immutable Collection as default values. You mant want to use a plain javascript object instead.' + ); + } + + if (defaultValues === null || typeof defaultValues !== 'object') { + throw new Error( + 'Can not call `Record` with a non-object as default values. You may want to use a javascript object.' + ); + } + + return defaultValues; +} export class Record { constructor(defaultValues, name) { let hasInitialized; + const _convertedDefaultValues = convertDefaultValues(defaultValues); + const RecordType = function Record(values) { if (values instanceof RecordType) { return values; @@ -42,16 +65,14 @@ export class Record { } if (!hasInitialized) { hasInitialized = true; - const keys = Object.keys( - isRecord(defaultValues) ? defaultValues.toObject() : defaultValues - ); + const keys = Object.keys(_convertedDefaultValues); const indices = (RecordTypePrototype._indices = {}); // Deprecated: left to attempt not to break any external code which // relies on a ._name property existing on record instances. // Use Record.getDescriptiveName() instead RecordTypePrototype._name = name; RecordTypePrototype._keys = keys; - RecordTypePrototype._defaultValues = defaultValues; + RecordTypePrototype._defaultValues = _convertedDefaultValues; for (let i = 0; i < keys.length; i++) { const propName = keys[i]; indices[propName] = i; From 3d150617813d3b05e3cf99e5586d1bd28103017b Mon Sep 17 00:00:00 2001 From: Julien Deniau Date: Fri, 20 Nov 2020 16:37:43 +0100 Subject: [PATCH 4/5] typo --- __tests__/__snapshots__/Record.ts.snap | 4 ++-- src/Record.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/__tests__/__snapshots__/Record.ts.snap b/__tests__/__snapshots__/Record.ts.snap index 401c03fb5e..1b35ad0327 100644 --- a/__tests__/__snapshots__/Record.ts.snap +++ b/__tests__/__snapshots__/Record.ts.snap @@ -1,5 +1,5 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`Record does not accept a non object as constructor 1`] = `"Can not call \`Record\` with a non-object as default values. You may want to use a javascript object."`; +exports[`Record does not accept a non object as constructor 1`] = `"Can not call \`Record\` with a non-object as default values. Use a plain javascript object instead."`; -exports[`Record does not accept an immutable object that is not a Record as constructor 1`] = `"Can not call \`Record\` an immutable Collection as default values. You mant want to use a plain javascript object instead."`; +exports[`Record does not accept an immutable object that is not a Record as constructor 1`] = `"Can not call \`Record\` with an immutable Collection as default values. Use a plain javascript object instead."`; diff --git a/src/Record.js b/src/Record.js index ade042d977..01b5024ff9 100644 --- a/src/Record.js +++ b/src/Record.js @@ -37,13 +37,13 @@ function convertDefaultValues(defaultValues) { if (isImmutable(defaultValues)) { throw new Error( - 'Can not call `Record` an immutable Collection as default values. You mant want to use a plain javascript object instead.' + 'Can not call `Record` with an immutable Collection as default values. Use a plain javascript object instead.' ); } if (defaultValues === null || typeof defaultValues !== 'object') { throw new Error( - 'Can not call `Record` with a non-object as default values. You may want to use a javascript object.' + 'Can not call `Record` with a non-object as default values. Use a plain javascript object instead.' ); } From 17394064095f02d6879eec69e35e4186e80299b1 Mon Sep 17 00:00:00 2001 From: Julien Deniau Date: Thu, 3 Dec 2020 15:16:38 +0100 Subject: [PATCH 5/5] throw on invalid default values instead of converting to object --- __tests__/Record.ts | 7 +++---- __tests__/__snapshots__/Record.ts.snap | 2 ++ src/Record.js | 14 +++++++------- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/__tests__/Record.ts b/__tests__/Record.ts index 1f528c9057..24bbf30f80 100644 --- a/__tests__/Record.ts +++ b/__tests__/Record.ts @@ -265,10 +265,9 @@ describe('Record', () => { it('does not accept a Record as constructor', () => { const Foo = Record({ foo: 'bar' }); const fooInstance = Foo(); - const Bar = Record(fooInstance); - const barInstance = Bar(); - - expect(barInstance.toJS()).toEqual(fooInstance.toJS()); + expect(() => { + Record(fooInstance); + }).toThrowErrorMatchingSnapshot(); }); it('does not accept a non object as constructor', () => { diff --git a/__tests__/__snapshots__/Record.ts.snap b/__tests__/__snapshots__/Record.ts.snap index 1b35ad0327..46daf60171 100644 --- a/__tests__/__snapshots__/Record.ts.snap +++ b/__tests__/__snapshots__/Record.ts.snap @@ -1,5 +1,7 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`Record does not accept a Record as constructor 1`] = `"Can not call \`Record\` with an immutable Record as default values. Use a plain javascript object instead."`; + exports[`Record does not accept a non object as constructor 1`] = `"Can not call \`Record\` with a non-object as default values. Use a plain javascript object instead."`; exports[`Record does not accept an immutable object that is not a Record as constructor 1`] = `"Can not call \`Record\` with an immutable Collection as default values. Use a plain javascript object instead."`; diff --git a/src/Record.js b/src/Record.js index 01b5024ff9..68ffec25c8 100644 --- a/src/Record.js +++ b/src/Record.js @@ -30,9 +30,11 @@ import invariant from './utils/invariant'; import quoteString from './utils/quoteString'; import { isImmutable } from './predicates/isImmutable'; -function convertDefaultValues(defaultValues) { +function throwOnInvalidDefaultValues(defaultValues) { if (isRecord(defaultValues)) { - return defaultValues.toObject(); + throw new Error( + 'Can not call `Record` with an immutable Record as default values. Use a plain javascript object instead.' + ); } if (isImmutable(defaultValues)) { @@ -46,15 +48,13 @@ function convertDefaultValues(defaultValues) { 'Can not call `Record` with a non-object as default values. Use a plain javascript object instead.' ); } - - return defaultValues; } export class Record { constructor(defaultValues, name) { let hasInitialized; - const _convertedDefaultValues = convertDefaultValues(defaultValues); + throwOnInvalidDefaultValues(defaultValues); const RecordType = function Record(values) { if (values instanceof RecordType) { @@ -65,14 +65,14 @@ export class Record { } if (!hasInitialized) { hasInitialized = true; - const keys = Object.keys(_convertedDefaultValues); + const keys = Object.keys(defaultValues); const indices = (RecordTypePrototype._indices = {}); // Deprecated: left to attempt not to break any external code which // relies on a ._name property existing on record instances. // Use Record.getDescriptiveName() instead RecordTypePrototype._name = name; RecordTypePrototype._keys = keys; - RecordTypePrototype._defaultValues = _convertedDefaultValues; + RecordTypePrototype._defaultValues = defaultValues; for (let i = 0; i < keys.length; i++) { const propName = keys[i]; indices[propName] = i;