diff --git a/.changeset/brave-bats-itch.md b/.changeset/brave-bats-itch.md new file mode 100644 index 000000000000..e8dfbefe7cdd --- /dev/null +++ b/.changeset/brave-bats-itch.md @@ -0,0 +1,9 @@ +--- +'@data-client/normalizr': minor +--- + +BREAKING CHANGE: Denormalize always transforms immutablejs entities into the class + +Previously using ImmutableJS structures when calling denormalize() would maintain +nested schemas as immutablejs structures still. Now everything is converted to normal JS. +This is how the types have always been specified. diff --git a/docs/core/concepts/normalization.md b/docs/core/concepts/normalization.md index 5a5136cd4335..ec1232cb5108 100644 --- a/docs/core/concepts/normalization.md +++ b/docs/core/concepts/normalization.md @@ -392,7 +392,7 @@ xychart-beta title "Denormalize Single Entity" x-axis [normalizr, "Data Client", "Data Client (cached)"] y-axis "Operations per second" 0 --> 5618500 - bar [212500, 1288500, 5618500] + bar [212500, 1341000, 5618500] ``` ```mermaid @@ -400,7 +400,7 @@ xychart-beta title "Denormalize Large List" x-axis [normalizr, "Data Client", "Data Client (cached)"] y-axis "Operations per second" 0 --> 12962 - bar [1151, 1807, 13182] + bar [1151, 1986, 13182] ``` diff --git a/examples/benchmark/old-normalizr/normalizr.js b/examples/benchmark/old-normalizr/normalizr.js index 964408961032..fcc649129887 100644 --- a/examples/benchmark/old-normalizr/normalizr.js +++ b/examples/benchmark/old-normalizr/normalizr.js @@ -46,8 +46,6 @@ function mergeWithStore({ entities, result }, storeState) { let curState = state; export default function addNormlizrSuite(suite) { - %OptimizeFunctionOnNextCall(normalize); - %OptimizeFunctionOnNextCall(denormalize); return suite .add('normalizeLong', () => { return mergeWithStore(normalize(data, ProjectSchema), state); @@ -63,7 +61,7 @@ export default function addNormlizrSuite(suite) { }) .add('denormalizeShort donotcache 500x', () => { for (let i = 0; i < 500; ++i) { - const user = denormalize('gnoff', User, githubState.entities); + var user = denormalize('gnoff', User, githubState.entities); // legacy normalizr doesn't convert this for us, so we must do manually afterward. user.createdAt = new Date(user.createdAt); user.updatedAt = new Date(user.updatedAt); diff --git a/packages/endpoint/src/schemas/__tests__/Entity.test.ts b/packages/endpoint/src/schemas/__tests__/Entity.test.ts index ef4ec247b0f4..23e8588d3a66 100644 --- a/packages/endpoint/src/schemas/__tests__/Entity.test.ts +++ b/packages/endpoint/src/schemas/__tests__/Entity.test.ts @@ -812,10 +812,8 @@ describe(`${Entity.name} denormalization`, () => { }, }; - expect(denormalize(Menu, '1', entities)).toMatchSnapshot(); expect(denormalize(Menu, '1', fromJS(entities))).toMatchSnapshot(); - expect(denormalize(Menu, '2', entities)).toMatchSnapshot(); expect(denormalize(Menu, '2', fromJS(entities))).toMatchSnapshot(); }); diff --git a/packages/endpoint/src/schemas/__tests__/EntityMixin.test.ts b/packages/endpoint/src/schemas/__tests__/EntityMixin.test.ts index eb1cdeee27af..121c9549594a 100644 --- a/packages/endpoint/src/schemas/__tests__/EntityMixin.test.ts +++ b/packages/endpoint/src/schemas/__tests__/EntityMixin.test.ts @@ -948,10 +948,8 @@ describe(`${schema.Entity.name} denormalization`, () => { }, }; - expect(denormalize(Menu, '1', entities)).toMatchSnapshot(); expect(denormalize(Menu, '1', fromJS(entities))).toMatchSnapshot(); - expect(denormalize(Menu, '2', entities)).toMatchSnapshot(); expect(denormalize(Menu, '2', fromJS(entities))).toMatchSnapshot(); }); diff --git a/packages/endpoint/src/schemas/__tests__/__snapshots__/Entity.test.ts.snap b/packages/endpoint/src/schemas/__tests__/__snapshots__/Entity.test.ts.snap index 77b8bd13d374..b08c3fae4818 100644 --- a/packages/endpoint/src/schemas/__tests__/__snapshots__/Entity.test.ts.snap +++ b/packages/endpoint/src/schemas/__tests__/__snapshots__/Entity.test.ts.snap @@ -60,22 +60,6 @@ Menu { `; exports[`Entity denormalization denormalizes deep entities with records 2`] = ` -Menu { - "food": Food { - "id": "1", - }, - "id": "1", -} -`; - -exports[`Entity denormalization denormalizes deep entities with records 3`] = ` -Menu { - "food": null, - "id": "2", -} -`; - -exports[`Entity denormalization denormalizes deep entities with records 4`] = ` Menu { "food": null, "id": "2", @@ -107,7 +91,7 @@ exports[`Entity denormalization denormalizes recursive dependencies 2`] = ` Report { "draftedBy": User { "id": "456", - "reports": Immutable.List [ + "reports": [ [Circular], ], "role": "manager", @@ -115,7 +99,7 @@ Report { "id": "123", "publishedBy": User { "id": "456", - "reports": Immutable.List [ + "reports": [ [Circular], ], "role": "manager", @@ -142,7 +126,7 @@ User { exports[`Entity denormalization denormalizes recursive dependencies 4`] = ` User { "id": "456", - "reports": Immutable.List [ + "reports": [ Report { "draftedBy": [Circular], "id": "123", diff --git a/packages/endpoint/src/schemas/__tests__/__snapshots__/EntityMixin.test.ts.snap b/packages/endpoint/src/schemas/__tests__/__snapshots__/EntityMixin.test.ts.snap index c536a0cec1b4..7870af8b6bda 100644 --- a/packages/endpoint/src/schemas/__tests__/__snapshots__/EntityMixin.test.ts.snap +++ b/packages/endpoint/src/schemas/__tests__/__snapshots__/EntityMixin.test.ts.snap @@ -75,22 +75,6 @@ Menu { `; exports[`EntityMixin denormalization denormalizes deep entities with records 2`] = ` -Menu { - "food": Food { - "id": "1", - }, - "id": "1", -} -`; - -exports[`EntityMixin denormalization denormalizes deep entities with records 3`] = ` -Menu { - "food": null, - "id": "2", -} -`; - -exports[`EntityMixin denormalization denormalizes deep entities with records 4`] = ` Menu { "food": null, "id": "2", @@ -148,7 +132,7 @@ exports[`EntityMixin denormalization nesting denormalizes recursive dependencies Report { "draftedBy": User { "id": "456", - "reports": Immutable.List [ + "reports": [ [Circular], ], "role": "manager", @@ -156,7 +140,7 @@ Report { "id": "123", "publishedBy": User { "id": "456", - "reports": Immutable.List [ + "reports": [ [Circular], ], "role": "manager", @@ -183,7 +167,7 @@ User { exports[`EntityMixin denormalization nesting denormalizes recursive dependencies 4`] = ` User { "id": "456", - "reports": Immutable.List [ + "reports": [ Report { "draftedBy": [Circular], "id": "123", diff --git a/packages/endpoint/src/schemas/__tests__/__snapshots__/Values.test.js.snap b/packages/endpoint/src/schemas/__tests__/__snapshots__/Values.test.js.snap index 6039fd221fd1..8ab9ab8cf119 100644 --- a/packages/endpoint/src/schemas/__tests__/__snapshots__/Values.test.js.snap +++ b/packages/endpoint/src/schemas/__tests__/__snapshots__/Values.test.js.snap @@ -461,69 +461,69 @@ exports[`input (immutable) ValuesSchema denormalization (current) works on compl "data": { "estimates": { "BTC": Estimate { - "coinbase_fees": Immutable.Map { + "coinbase_fees": { "amount": "0.00002270", "currency": "BTC", }, "confirmation_duration": 900, - "exchange": Immutable.Map { - "rate": "6820.07", - "local": "USD", + "exchange": { "crypto": "BTC", - }, - "exchange_to_proceeds": Immutable.Map { + "local": "USD", "rate": "6820.07", - "local": "EUR", + }, + "exchange_to_proceeds": { "crypto": "BTC", + "local": "EUR", + "rate": "6820.07", }, - "fee": Immutable.Map { + "fee": { "amount": "0.00002270", "currency": "BTC", }, - "fee_per_kb": Immutable.Map { + "fee_per_kb": { "amount": "0.00016566", "currency": "BTC", }, - "min_order_size": Immutable.Map { + "min_order_size": { "amount": "0.001", "currency": "BTC", }, "priority": "fast", - "recipient_value": Immutable.Map { + "recipient_value": { "amount": "0.00054147", "currency": "BTC", }, }, "ETH": Estimate { - "coinbase_fees": Immutable.Map { + "coinbase_fees": { "amount": "0.00002270", "currency": "BTC", }, "confirmation_duration": 900, - "exchange": Immutable.Map { - "rate": "197.07", - "local": "USD", + "exchange": { "crypto": "ETH", + "local": "USD", + "rate": "197.07", }, - "exchange_to_proceeds": Immutable.Map { - "rate": "6820.07", - "local": "EUR", + "exchange_to_proceeds": { "crypto": "BTC", + "local": "EUR", + "rate": "6820.07", }, - "fee": Immutable.Map { + "fee": { "amount": "0.03795", "currency": "ETH", }, - "fee_per_kb": Immutable.Map { + "fee_per_kb": { "amount": "0.00086", "currency": "ETH", }, - "min_order_size": Immutable.Map { + "min_order_size": { "amount": "0.001", "currency": "BTC", }, "priority": "fast", - "recipient_value": Immutable.Map { + "recipient_value": { "amount": "2.53", "currency": "ETH", }, diff --git a/packages/normalizr/src/denormalize/denormalize.ts b/packages/normalizr/src/denormalize/denormalize.ts index b831f7f31129..aefe30ae8fe6 100644 --- a/packages/normalizr/src/denormalize/denormalize.ts +++ b/packages/normalizr/src/denormalize/denormalize.ts @@ -2,6 +2,7 @@ import { getEntities } from './getEntities.js'; import LocalCache from './localCache.js'; import getUnvisit from './unvisit.js'; import type { Schema } from '../interface.js'; +import { isImmutable } from '../schemas/ImmutableUtils.js'; import type { DenormalizeNullable } from '../types.js'; export function denormalize( @@ -19,5 +20,6 @@ export function denormalize( getEntities(entities), new LocalCache(), args, + isImmutable(entities), )(schema, input).data; } diff --git a/packages/normalizr/src/denormalize/unvisit.ts b/packages/normalizr/src/denormalize/unvisit.ts index 38b694669ce6..2d4edfcd1b8b 100644 --- a/packages/normalizr/src/denormalize/unvisit.ts +++ b/packages/normalizr/src/denormalize/unvisit.ts @@ -5,109 +5,133 @@ import { UNDEF } from './UNDEF.js'; import type { EntityInterface } from '../interface.js'; import { isEntity } from '../isEntity.js'; import { denormalize as arrayDenormalize } from '../schemas/Array.js'; -import { isImmutable } from '../schemas/ImmutableUtils.js'; import { denormalize as objectDenormalize } from '../schemas/Object.js'; import type { EntityPath } from '../types.js'; -function unvisitEntity( - schema: EntityInterface, - entityOrId: Record | string, - args: readonly any[], - unvisit: (schema: any, input: any) => any, +const getUnvisitEntity = ( getEntity: GetEntity, cache: Cache, -): object | undefined | symbol { - const entity = - typeof entityOrId === 'object' ? entityOrId : ( - getEntity({ key: schema.key, pk: entityOrId }) - ); - if (typeof entity === 'symbol' && typeof schema.denormalize === 'function') { - return schema.denormalize(entity, args, unvisit); - } - - if ( - entity === undefined && - typeof entityOrId !== 'object' && - entityOrId !== '' && - entityOrId !== 'undefined' - ) { - // we cannot perform lookups with `undefined`, so we use a special object to represent undefined - // we're actually using this call to ensure we update the cache if a nested schema changes from `undefined` - // this is because cache.getEntity adds this key,pk as a dependency of anything it is nested under - return cache.getEntity(entityOrId, schema, UNDEF, localCacheKey => { - localCacheKey.set(entityOrId, undefined); - }); - } - - if (typeof entity !== 'object' || entity === null) { - return entity as any; - } + args: readonly any[], + isImmutable: boolean, + unvisit: (schema: any, input: any) => any, +) => { + return function unvisitEntity( + schema: EntityInterface, + entityOrId: Record | string, + ): object | undefined | symbol { + const inputIsId = typeof entityOrId !== 'object'; + const entity = + inputIsId ? getEntity({ key: schema.key, pk: entityOrId }) : entityOrId; + if (typeof entity === 'symbol') { + return schema.denormalize(entity, args, unvisit); + } - let pk: string = - typeof entityOrId !== 'object' ? entityOrId : ( - (schema.pk( - isImmutable(entity) ? (entity as any).toJS() : entity, - undefined, - undefined, - args, - ) as any) - ); + if ( + entity === undefined && + inputIsId && + // entityOrId cannot be undefined literal as this function wouldn't be called in that case + // however the blank strings can still occur + entityOrId !== '' && + entityOrId !== 'undefined' + ) { + // we cannot perform WeakMap lookups with `undefined`, so we use a special object to represent undefined + // we're actually using this call to ensure we update the cache if a nested schema changes from `undefined` + // this is because cache.getEntity adds this key,pk as a dependency of anything it is nested under + return cache.getEntity(entityOrId, schema, UNDEF, localCacheKey => { + localCacheKey.set(entityOrId, undefined); + }); + } - // if we can't generate a working pk we cannot do cache lookups properly, - // so simply denormalize without caching - if (pk === undefined || pk === '' || pk === 'undefined') { - return noCacheGetEntity(localCacheKey => - unvisitEntityObject(entity, schema, unvisit, '', localCacheKey, args), - ); - } + if (typeof entity !== 'object' || entity === null) { + return entity as any; + } - // just an optimization to make all cache usages of pk monomorphic - if (typeof pk !== 'string') pk = `${pk}`; + const entityObject: object = + isImmutable && entityOrId !== entity ? + ((entity as any).toJS() as object) + : entity; + + let pk: string | number | undefined = + inputIsId ? entityOrId : ( + (schema.pk(entityObject, undefined, undefined, args) as any) + ); + + // if we can't generate a working pk we cannot do cache lookups properly, + // so simply denormalize without caching + if (pk === undefined || pk === '' || pk === 'undefined') { + return noCacheGetEntity(localCacheKey => + unvisitEntityObject( + schema, + entityObject, + '', + localCacheKey, + args, + unvisit, + ), + ); + } - // last function computes if it is not in any caches - return cache.getEntity(pk, schema, entity, localCacheKey => - unvisitEntityObject(entity, schema, unvisit, pk, localCacheKey, args), - ); -} + // just an optimization to make all cache usages of pk monomorphic + if (typeof pk !== 'string') pk = `${pk}`; -function noCacheGetEntity( - computeValue: (localCacheKey: Map) => void, -): object | undefined | symbol { - const localCacheKey = new Map(); - computeValue(localCacheKey); - - return localCacheKey.get(''); -} + // last function computes if it is not in any caches + return cache.getEntity(pk, schema, entityObject, localCacheKey => + unvisitEntityObject( + schema, + entityObject, + pk, + localCacheKey, + args, + unvisit, + ), + ); + }; +}; function unvisitEntityObject( - entity: object, schema: EntityInterface, - unvisit: (schema: any, input: any) => any, + entity: object, pk: string, localCacheKey: Map, args: readonly any[], + unvisit: (schema: any, input: any) => any, ): void { - const entityCopy = - isImmutable(entity) ? - schema.createIfValid(entity.toObject()) - : schema.createIfValid(entity); - localCacheKey.set(pk, entityCopy); + const entityCopy = schema.createIfValid(entity); if (entityCopy === undefined) { // undefined indicates we should suspense (perhaps failed validation) localCacheKey.set(pk, INVALID); } else { - if (typeof schema.denormalize === 'function') { - localCacheKey.set(pk, schema.denormalize(entityCopy, args, unvisit)); - } + // set before we recurse to prevent cycles causing infinite loops + localCacheKey.set(pk, entityCopy); + // we still need to set in case denormalize recursively finds INVALID + localCacheKey.set(pk, schema.denormalize(entityCopy, args, unvisit)); } } +function noCacheGetEntity( + computeValue: (localCacheKey: Map) => void, +): object | undefined | symbol { + const localCacheKey = new Map(); + computeValue(localCacheKey); + + return localCacheKey.get(''); +} + const getUnvisit = ( getEntity: GetEntity, cache: Cache, args: readonly any[], + isImmutable: boolean, ) => { + // we don't inline this as making this function too big inhibits v8's JIT + const unvisitEntity = getUnvisitEntity( + getEntity, + cache, + args, + isImmutable, + unvisit, + ); function unvisit(schema: any, input: any): any { if (!schema) return input; @@ -129,7 +153,7 @@ const getUnvisit = ( } } else { if (isEntity(schema)) { - return unvisitEntity(schema, input, args, unvisit, getEntity, cache); + return unvisitEntity(schema, input); } return schema.denormalize(input, args, unvisit); diff --git a/packages/normalizr/src/memo/MemoCache.ts b/packages/normalizr/src/memo/MemoCache.ts index 9ac694a24178..40486e2bffc0 100644 --- a/packages/normalizr/src/memo/MemoCache.ts +++ b/packages/normalizr/src/memo/MemoCache.ts @@ -50,6 +50,7 @@ export default class MemoCache { getEntity, new GlobalCache(getEntity, this.entities, this.endpoints), args, + isImmutable(entities), )(schema, input); }