Merge pull request #7621 from QwikDev/v2-throw-on-serializing-function

feat: throw on serializing function

Author: Varixo

.changeset/rare-candies-join.md

@@ -0,0 +1,5 @@
+---
+'@qwik.dev/core': patch
+---
+
+fix: skip serialize functions wrapped with the `noSerialize`

packages/qwik/src/core/shared/error/error.ts

@@ -9,53 +9,37 @@ export const codeToText = (code: number, ...parts: any[]): string => {
       'Scheduler not found', // 1
       'track() received object, without prop to track', // 2
       'Only primitive and object literals can be serialized. {{0}}', // 3
-      '', // 4 unused
-      'You can render over a existing q:container. Skipping render().', // 5
-      '', // 6 unused
-      '', // 7 unused
-      '', // 8 unused
-      '', // 9 unused
-      'QRL is not a function', // 10
-      'Dynamic import not found', // 11
-      'Unknown type argument', // 12
-      `Actual value for useContext({{0}}) can not be found, make sure some ancestor component has set a value using useContextProvider(). In the browser make sure that the context was used during SSR so its state was serialized.`, // 13
-      "Invoking 'use*()' method outside of invocation context.", // 14
-      '', // 15 unused
-      '', // 16 unused
-      '', // 17 unused
-      '', // 18 unused
-      '', // 19 unused
-      `Calling a 'use*()' method outside 'component$(() => { HERE })' is not allowed. 'use*()' methods provide hooks to the 'component$' state and lifecycle, ie 'use' hooks can only be called synchronously within the 'component$' function or another 'use' method.\nSee https://qwik.dev/docs/components/tasks/#use-method-rules`, // 20
-      '', // 21 unused
-      '', // 22 unused
-      '', // 23 unused
-      '', // 24 unused
-      '', // 25 unused
-      '', // 26 unused
-      '', // 27 unused
-      'The provided Context reference "{{0}}" is not a valid context created by createContextId()', // 28
-      'SsrError(tag): {{0}}', // 29
-      'QRLs can not be resolved because it does not have an attached container. This means that the QRL does not know where it belongs inside the DOM, so it cant dynamically import() from a relative path.', // 30
-      'QRLs can not be dynamically resolved, because it does not have a chunk path', // 31
-      '{{0}}\nThe JSX ref attribute must be a Signal', // 32
-      'Serialization Error: Deserialization of data type {{0}} is not implemented', // 33
-      'Serialization Error: Expected vnode for ref prop, but got {{0}}', // 34
-      'Serialization Error: Cannot allocate data type {{0}}', // 35
-      'Serialization Error: Missing root id for {{0}}', // 36
-      'Serialization Error: Serialization of data type {{0}} is not implemented', // 37
-      'Serialization Error: Unvisited {{0}}', // 38
-      'Serialization Error: Missing QRL chunk for {{0}}', // 39
-      '{{0}}\nThe value of the textarea must be a string found {{1}}', // 40
-      'Unable to find q:container', // 41
-      "Element must have 'q:container' attribute.", // 42
-      'Unknown vnode type {{0}}.', // 43
-      'Materialize error: missing element: {{0}} {{1}} {{2}}', // 44
-      'Cannot coerce a Signal, use `.value` instead', // 45
-      'useComputed$ QRL {{0}} {{1}} cannot return a Promise', // 46
-      'ComputedSignal is read-only', // 47
-      'WrappedSignal is read-only', // 48
-      'Attribute value is unsafe for SSR', // 49
-      'SerializerSymbol function returned rejected promise', // 50
+      'You can render over a existing q:container. Skipping render().', // 4
+      'QRL is not a function', // 5
+      'Dynamic import not found', // 6
+      'Unknown type argument', // 7
+      `Actual value for useContext({{0}}) can not be found, make sure some ancestor component has set a value using useContextProvider(). In the browser make sure that the context was used during SSR so its state was serialized.`, // 8
+      "Invoking 'use*()' method outside of invocation context.", // 9
+      `Calling a 'use*()' method outside 'component$(() => { HERE })' is not allowed. 'use*()' methods provide hooks to the 'component$' state and lifecycle, ie 'use' hooks can only be called synchronously within the 'component$' function or another 'use' method.\nSee https://qwik.dev/docs/components/tasks/#use-method-rules`, // 10
+      'The provided Context reference "{{0}}" is not a valid context created by createContextId()', // 11
+      'SsrError(tag): {{0}}', // 12
+      'QRLs can not be resolved because it does not have an attached container. This means that the QRL does not know where it belongs inside the DOM, so it cant dynamically import() from a relative path.', // 13
+      'QRLs can not be dynamically resolved, because it does not have a chunk path', // 14
+      '{{0}}\nThe JSX ref attribute must be a Signal', // 15
+      'Serialization Error: Deserialization of data type {{0}} is not implemented', // 16
+      'Serialization Error: Expected vnode for ref prop, but got {{0}}', // 17
+      'Serialization Error: Cannot allocate data type {{0}}', // 18
+      'Serialization Error: Missing root id for {{0}}', // 19
+      'Serialization Error: Serialization of data type {{0}} is not implemented', // 20
+      'Serialization Error: Unvisited {{0}}', // 21
+      'Serialization Error: Missing QRL chunk for {{0}}', // 22
+      '{{0}}\nThe value of the textarea must be a string found {{1}}', // 23
+      'Unable to find q:container', // 24
+      "Element must have 'q:container' attribute.", // 25
+      'Unknown vnode type {{0}}.', // 26
+      'Materialize error: missing element: {{0}} {{1}} {{2}}', // 27
+      'Cannot coerce a Signal, use `.value` instead', // 28
+      'useComputed$ QRL {{0}} {{1}} cannot return a Promise', // 29
+      'ComputedSignal is read-only', // 30
+      'WrappedSignal is read-only', // 31
+      'Attribute value is unsafe for SSR', // 32
+      'SerializerSymbol function returned rejected promise', // 33
+      'Serialization Error: Cannot serialize function: {{0}}', // 34
     ];
     let text = MAP[code] ?? '';
     if (parts.length) {
@@ -79,53 +63,37 @@ export const enum QError {
   schedulerNotFound = 1,
   trackObjectWithoutProp = 2,
   verifySerializable = 3,
-  UNUSED_4 = 4,
-  cannotRenderOverExistingContainer = 5,
-  UNUSED_6 = 6,
-  UNUSED_7 = 7,
-  UNUSED_8 = 8,
-  UNUSED_9 = 9,
-  qrlIsNotFunction = 10,
-  dynamicImportFailed = 11,
-  unknownTypeArgument = 12,
-  notFoundContext = 13,
-  useMethodOutsideContext = 14,
-  UNUSED_15 = 15,
-  UNUSED_16 = 16,
-  UNUSED_17 = 17,
-  UNUSED_18 = 18,
-  UNUSED_19 = 19,
-  useInvokeContext = 20,
-  UNUSED_21 = 21,
-  UNUSED_22 = 22,
-  UNUSED_23 = 23,
-  UNUSED_24 = 24,
-  UNUSED_25 = 25,
-  UNUSED_26 = 26,
-  UNUSED_27 = 27,
-  invalidContext = 28,
-  tagError = 29,
-  qrlMissingContainer = 30,
-  qrlMissingChunk = 31,
-  invalidRefValue = 32,
-  serializeErrorNotImplemented = 33,
-  serializeErrorExpectedVNode = 34,
-  serializeErrorCannotAllocate = 35,
-  serializeErrorMissingRootId = 36,
-  serializeErrorUnknownType = 37,
-  serializeErrorUnvisited = 38,
-  serializeErrorMissingChunk = 39,
-  wrongTextareaValue = 40,
-  containerNotFound = 41,
-  elementWithoutContainer = 42,
-  invalidVNodeType = 43,
-  materializeVNodeDataError = 44,
-  cannotCoerceSignal = 45,
-  computedNotSync = 46,
-  computedReadOnly = 47,
-  wrappedReadOnly = 48,
-  unsafeAttr = 49,
-  serializerSymbolRejectedPromise = 50,
+  cannotRenderOverExistingContainer = 4,
+  qrlIsNotFunction = 5,
+  dynamicImportFailed = 6,
+  unknownTypeArgument = 7,
+  notFoundContext = 8,
+  useMethodOutsideContext = 9,
+  useInvokeContext = 10,
+  invalidContext = 11,
+  tagError = 12,
+  qrlMissingContainer = 13,
+  qrlMissingChunk = 14,
+  invalidRefValue = 15,
+  serializeErrorNotImplemented = 16,
+  serializeErrorExpectedVNode = 17,
+  serializeErrorCannotAllocate = 18,
+  serializeErrorMissingRootId = 19,
+  serializeErrorUnknownType = 20,
+  serializeErrorUnvisited = 21,
+  serializeErrorMissingChunk = 22,
+  wrongTextareaValue = 23,
+  containerNotFound = 24,
+  elementWithoutContainer = 25,
+  invalidVNodeType = 26,
+  materializeVNodeDataError = 27,
+  cannotCoerceSignal = 28,
+  computedNotSync = 29,
+  computedReadOnly = 30,
+  wrappedReadOnly = 31,
+  unsafeAttr = 32,
+  serializerSymbolRejectedPromise = 33,
+  serializeErrorCannotSerializeFunction = 34,
 }
 
 export const qError = (code: number, errorMessageArgs: any[] = []): Error => {

packages/qwik/src/core/shared/shared-serialization.ts

@@ -933,20 +933,21 @@ async function serialize(serializationContext: SerializationContext): Promise<vo
     serializationContext.$addRoot$(qrl, null);
   };
 
-  const outputRootRef = (value: unknown, elseCallback: () => void) => {
+  const outputRootRef = (value: unknown, rootDepth = 0) => {
     const seen = $wasSeen$(value);
     const rootRefPath = $pathMap$.get(value);
-    if (isRootObject() && seen && seen.$parent$ !== null && rootRefPath) {
+    if (rootDepth === depth && seen && seen.$parent$ !== null && rootRefPath) {
       output(TypeIds.RootRef, rootRefPath);
-    } else if (depth > 0 && seen && seen.$rootIndex$ !== -1) {
+      return true;
+    } else if (depth > rootDepth && seen && seen.$rootIndex$ !== -1) {
       output(TypeIds.RootRef, seen.$rootIndex$);
-    } else {
-      elseCallback();
+      return true;
     }
+    return false;
   };
 
   const writeValue = (value: unknown) => {
-    if (fastSkipSerialize(value as object)) {
+    if (fastSkipSerialize(value as object | Function)) {
       output(TypeIds.Constant, Constants.Undefined);
     } else if (typeof value === 'bigint') {
       output(TypeIds.BigInt, value.toString());
@@ -958,7 +959,7 @@ async function serialize(serializationContext: SerializationContext): Promise<vo
       } else if (value === Fragment) {
         output(TypeIds.Constant, Constants.Fragment);
       } else if (isQrl(value)) {
-        outputRootRef(value, () => {
+        if (!outputRootRef(value)) {
           const qrl = qrlToString(serializationContext, value);
           const type = preloadQrls.has(value) ? TypeIds.PreloadQRL : TypeIds.QRL;
           if (isRootObject()) {
@@ -967,15 +968,13 @@ async function serialize(serializationContext: SerializationContext): Promise<vo
             const id = serializationContext.$addRoot$(qrl);
             output(type, id);
           }
-        });
+        }
       } else if (isQwikComponent(value)) {
         const [qrl]: [QRLInternal] = (value as any)[SERIALIZABLE_STATE];
         serializationContext.$renderSymbols$.add(qrl.$symbol$);
         output(TypeIds.Component, [qrl]);
       } else {
-        // TODO this happens for inline components with render props like Resource
-        console.error('Cannot serialize function (ignoring for now): ' + value.toString());
-        output(TypeIds.Constant, Constants.Undefined);
+        throw qError(QError.serializeErrorCannotSerializeFunction, [value.toString()]);
       }
     } else if (typeof value === 'number') {
       if (Number.isNaN(value)) {
@@ -1013,9 +1012,9 @@ async function serialize(serializationContext: SerializationContext): Promise<vo
       if (value.length === 0) {
         output(TypeIds.Constant, Constants.EmptyString);
       } else {
-        outputRootRef(value, () => {
+        if (!outputRootRef(value)) {
           output(TypeIds.String, value);
-        });
+        }
       }
     } else if (typeof value === 'undefined') {
       output(TypeIds.Constant, Constants.Undefined);
@@ -1033,28 +1032,15 @@ async function serialize(serializationContext: SerializationContext): Promise<vo
      * The object writer outputs an array object (without type prefix) and this increases the depth
      * for the objects within (depth 1).
      */
-    const isRootObject = depth === 1;
     // Objects are the only way to create circular dependencies.
     // So the first thing to to is to see if we have a circular dependency.
     // (NOTE: For root objects we need to serialize them regardless if we have seen
     //        them before, otherwise the root object reference will point to itself.)
     // Also note that depth will be 1 for objects in root
-    if (isRootObject) {
-      const seen = $wasSeen$(value);
-      const rootPath = $pathMap$.get(value);
-      if (rootPath && seen && seen.$parent$ !== null) {
-        output(TypeIds.RootRef, rootPath);
-        return;
-      }
-    } else if (depth > 1) {
-      const seen = $wasSeen$(value);
-      if (seen && seen.$rootIndex$ !== -1) {
-        // We have seen this object before, so we can serialize it as a reference.
-        // Otherwise serialize as normal
-        output(TypeIds.RootRef, seen.$rootIndex$);
-        return;
-      }
+    if (outputRootRef(value, 1)) {
+      return;
     }
+
     if (isPropsProxy(value)) {
       const varProps = value[_VAR_PROPS];
       const constProps = value[_CONST_PROPS];

packages/qwik/src/core/shared/shared-serialization.unit.ts

@@ -26,6 +26,7 @@ import { isQrl } from './qrl/qrl-utils';
 import { NoSerializeSymbol, SerializerSymbol } from './utils/serialize-utils';
 import { SubscriptionData } from '../reactive-primitives/subscription-data';
 import { StoreFlags } from '../reactive-primitives/types';
+import { QError } from './error/error';
 
 const DEBUG = false;
 
@@ -942,6 +943,32 @@ describe('shared-serialization', () => {
         (5 chars)"
       `);
     });
+    it('should ignore functions in noSerialize set', async () => {
+      const obj = { hi: true, ignore: noSerialize(() => console.warn()) };
+      const state = await serialize(obj);
+      expect(dumpState(state)).toMatchInlineSnapshot(`
+        "
+        0 Object [
+          String "hi"
+          Constant true
+        ]
+        (17 chars)"
+      `);
+    });
+    it('should ignore functions with NoSerializeSymbol', async () => {
+      const ignore = () => console.warn();
+      (ignore as any)[NoSerializeSymbol] = true;
+      const obj = { hi: true, ignore };
+      const state = await serialize(obj);
+      expect(dumpState(state)).toMatchInlineSnapshot(`
+        "
+        0 Object [
+          String "hi"
+          Constant true
+        ]
+        (17 chars)"
+      `);
+    });
     it('should ignore NoSerializeSymbol', async () => {
       const obj = { hi: true, [NoSerializeSymbol]: true };
       const state = await serialize(obj);
@@ -1047,7 +1074,9 @@ describe('shared-serialization', () => {
         throw 'oh no';
       }
     }
-    await expect(serialize(new Foo())).rejects.toThrow('Q50');
+    await expect(serialize(new Foo())).rejects.toThrow(
+      'Q' + QError.serializerSymbolRejectedPromise
+    );
     expect(consoleSpy).toHaveBeenCalledWith('oh no');
     consoleSpy.mockRestore();
   });

packages/qwik/src/core/shared/utils/serialize-utils.ts

@@ -99,8 +99,12 @@ export const shouldSerialize = (obj: unknown): boolean => {
   return true;
 };
 
-export const fastSkipSerialize = (obj: object): boolean => {
-  return typeof obj === 'object' && obj && (NoSerializeSymbol in obj || noSerializeSet.has(obj));
+export const fastSkipSerialize = (obj: object | Function): boolean => {
+  return (
+    obj &&
+    (typeof obj === 'object' || typeof obj === 'function') &&
+    (NoSerializeSymbol in obj || noSerializeSet.has(obj))
+  );
 };
 
 export const fastWeakSerialize = (obj: object): boolean => {