feat: improve error messages (QwikDev#2679)

youssmak · Jan 20, 2023 · b5a8bc9 · b5a8bc9
1 parent fefdb72
commit b5a8bc9
Show file tree

Hide file tree

Showing 5 changed files with 85 additions and 14 deletions.
diff --git a/packages/qwik/src/core/error/error.ts b/packages/qwik/src/core/error/error.ts
@@ -62,7 +62,8 @@ export const codeToText = (code: number): string => {
       'props are immutable', // 17
       '<div> component can only be used at the root of a Qwik component$()', // 18
       'Props are immutable by default.', // 19
-      'use- method must be called only at the root level of a component$()', // 20
+      `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 syncronously within the 'component$' function or another 'use' method.
+For more information see: https://qwik.builder.io/docs/components/lifecycle/#use-method-rules`, // 20
       'Container is already paused. Skipping', // 21
       'Components using useServerMount() can only be mounted in the server, if you need your component to be mounted in the client, use "useMount$()" instead', // 22
       'When rendering directly on top of Document, the root node must be a <html>', // 23

diff --git a/packages/qwik/src/core/render/dom/render.unit.tsx b/packages/qwik/src/core/render/dom/render.unit.tsx
@@ -38,7 +38,6 @@ renderSuite('should only render string/number', async () => {
       {null}
       {undefined}
       {[]}
-      {function () {}}
     </div>
   );
   await expectRendered(fixture, '<div>string123</div>');

diff --git a/packages/qwik/src/core/render/jsx/jsx-runtime.ts b/packages/qwik/src/core/render/jsx/jsx-runtime.ts
@@ -8,6 +8,9 @@ import { isQrl } from '../../qrl/qrl-class';
 import { invoke } from '../../use/use-core';
 import { verifySerializable } from '../../state/common';
 import { isQwikComponent } from '../../component/component.public';
+import { isSignal } from '../../state/signal';
+import { isPromise } from '../../util/promises';
+import { SkipRender } from './utils.public';
 
 let warnClassname = false;
 
@@ -36,12 +39,38 @@ export class JSXNodeImpl<T> implements JSXNode<T> {
   ) {
     if (qDev) {
       invoke(undefined, () => {
+        const isQwikC = isQwikComponent(type);
         if (!isString(type) && !isFunction(type)) {
           throw qError(QError_invalidJsxNodeType, type);
         }
         if (isArray((props as any).children)) {
+          const flatChildren = (props as any).children.flat();
+          if (isString(type) || isQwikC) {
+            flatChildren.forEach((child: any) => {
+              if (!isValidJSXChild(child)) {
+                const typeObj = typeof child;
+                let explanation = '';
+                if (typeObj === 'object') {
+                  if (child?.constructor) {
+                    explanation = `it's an instance of "${child?.constructor.name}".`;
+                  } else {
+                    explanation = `it's a object literal: ${printObjectLiteral(child)} `;
+                  }
+                } else if (typeObj === 'function') {
+                  explanation += `it's a function named "${(child as Function).name}".`;
+                } else {
+                  explanation = `it's a "${typeObj}": ${String(child)}.`;
+                }
+
+                throw createJSXError(
+                  `One of the children of <${type} /> is not an accepted value. JSX children must be either: string, boolean, number, <element>, Array, undefined/null, or a Promise/Signal that resolves to one of those types. Instead, ${explanation}`,
+                  this as any
+                );
+              }
+            });
+          }
           const keys: Record<string, boolean> = {};
-          (props as any).children.flat().forEach((child: any) => {
+          flatChildren.forEach((child: any) => {
             if (isJSXNode(child) && child.key != null) {
               if (keys[child.key]) {
                 const err = createJSXError(
@@ -65,27 +94,50 @@ export class JSXNodeImpl<T> implements JSXNode<T> {
                 throw qError(QError_invalidJsxNodeType, type);
               }
             }
-            if (prop !== 'children' && isQwikComponent(type) && value) {
+            if (prop !== 'children' && isQwikC && value) {
               verifySerializable(
                 value,
                 `The value of the JSX property "${prop}" can not be serialized`
               );
             }
           }
         }
+        if (isString(type)) {
+          if (type === 'style') {
+            if ((props as any).children) {
+              logWarn(`jsx: Using <style>{content}</style> will escape the content, effectively breaking the CSS.
+In order to disable content escaping use '<style dangerouslySetInnerHTML={content}/>'
+
+However, if the use case is to inject component styleContent, use 'useStyles$()' instead, it will be a lot more efficient.
+See https://qwik.builder.io/docs/components/styles/#usestyles for more information.`);
+            }
+          }
+          if (type === 'script') {
+            if ((props as any).children) {
+              logWarn(`jsx: Using <script>{content}</script> will escape the content, effectively breaking the inlined JS.
+In order to disable content escaping use '<script dangerouslySetInnerHTML={content}/>'`);
+            }
+          }
+          if ('className' in (props as any)) {
+            (props as any)['class'] = (props as any)['className'];
+            delete (props as any)['className'];
+            if (qDev && !warnClassname) {
+              warnClassname = true;
+              logWarn('jsx: `className` is deprecated. Use `class` instead.');
+            }
+          }
+        }
       });
     }
-    if (typeof type === 'string' && 'className' in (props as any)) {
-      (props as any)['class'] = (props as any)['className'];
-      delete (props as any)['className'];
-      if (qDev && !warnClassname) {
-        warnClassname = true;
-        logWarn('jsx: `className` is deprecated. Use `class` instead.');
-      }
-    }
   }
 }
 
+const printObjectLiteral = (obj: Record<string, any>) => {
+  return `{ ${Object.keys(obj)
+    .map((key) => `"${key}"`)
+    .join(', ')} }`;
+};
+
 export const isJSXNode = (n: any): n is JSXNode => {
   if (qDev) {
     if (n instanceof JSXNodeImpl) {
@@ -101,6 +153,24 @@ export const isJSXNode = (n: any): n is JSXNode => {
   }
 };
 
+export const isValidJSXChild = (node: any): boolean => {
+  if (!node) {
+    return true;
+  } else if (node === SkipRender) {
+    return true;
+  } else if (isString(node) || typeof node === 'number' || typeof node === 'boolean') {
+    return true;
+  } else if (isJSXNode(node)) {
+    return true;
+  }
+  if (isSignal(node)) {
+    return isValidJSXChild(node.value);
+  } else if (isPromise(node)) {
+    return true;
+  }
+  return false;
+};
+
 /**
  * @public
  */

diff --git a/packages/qwik/src/core/render/ssr/render-ssr.ts b/packages/qwik/src/core/render/ssr/render-ssr.ts
@@ -788,6 +788,7 @@ const processData = (
     return node.then((node) => processData(node, rCtx, ssrCtx, stream, flags, beforeClose));
   } else {
     logWarn('A unsupported value was passed to the JSX, skipping render. Value:', node);
+    return;
   }
 };
 

diff --git a/packages/qwik/src/core/use/use-core.ts b/packages/qwik/src/core/use/use-core.ts
@@ -71,8 +71,8 @@ export const getInvokeContext = (): InvokeContext => {
 };
 
 export const useInvokeContext = (): RenderInvokeContext => {
-  const ctx = getInvokeContext();
-  if (ctx.$event$ !== RenderEvent) {
+  const ctx = tryGetInvokeContext();
+  if (!ctx || ctx.$event$ !== RenderEvent) {
     throw qError(QError_useInvokeContext);
   }
   assertDefined(ctx.$hostElement$, `invoke: $hostElement$ must be defined`, ctx);
-Original file line number
+Diff line change
@@ Expand Up / @@ -38,7 +38,6 @@ renderSuite('should only render string/number', async () => { @@
           {null}
           {undefined}
           {[]}
-          {function () {}}
         </div>
       );
       await expectRendered(fixture, '<div>string123</div>');
@@ Expand Down @@