fix(#40388): next/dynamic should only add default loading without s…

…uspense (#40397)

The PR fixes #40388.

Currently, `next/dynamic` will try to provide a default `loading` to the
`loadableOptions` even when `suspense` is enabled, thus triggering the
incorrect warning. The PR fixes that. The corresponding integration test
case is also updated.

cc @huozhi 

## Bug

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

Co-authored-by: huozhi <inbox@huozhi.im>

packages/next/shared/lib/dynamic.tsx

@@ -65,29 +65,33 @@ export default function dynamic<P = {}>(
   options?: DynamicOptions<P>
 ): React.ComponentType<P> {
   let loadableFn: LoadableFn<P> = Loadable
-  let loadableOptions: LoadableOptions<P> = {
-    // A loading component is not required, so we default it
-    loading: ({ error, isLoading, pastDelay }) => {
-      if (!pastDelay) return null
-      if (process.env.NODE_ENV === 'development') {
-        if (isLoading) {
+
+  let loadableOptions: LoadableOptions<P> = options?.suspense
+    ? {}
+    : // only provide a default loading component when suspense is disabled
+      {
+        // A loading component is not required, so we default it
+        loading: ({ error, isLoading, pastDelay }) => {
+          if (!pastDelay) return null
+          if (process.env.NODE_ENV === 'development') {
+            if (isLoading) {
+              return null
+            }
+            if (error) {
+              return (
+                <p>
+                  {error.message}
+                  <br />
+                  {error.stack}
+                </p>
+              )
+            }
+          }
+
           return null
-        }
-        if (error) {
-          return (
-            <p>
-              {error.message}
-              <br />
-              {error.stack}
-            </p>
-          )
-        }
+        },
       }
 
-      return null
-    },
-  }
-
   // Support for direct import(), eg: dynamic(import('../hello-world'))
   // Note that this is only kept for the edge case where someone is passing in a promise as first argument
   // The react-loadable babel plugin will turn dynamic(import('../hello-world')) into dynamic(() => import('../hello-world'))
@@ -112,8 +116,8 @@ export default function dynamic<P = {}>(
     )
   }
 
-  if (process.env.NODE_ENV !== 'production') {
-    if (loadableOptions.suspense) {
+  if (loadableOptions.suspense) {
+    if (process.env.NODE_ENV !== 'production') {
       /**
        * TODO: Currently, next/dynamic will opt-in to React.lazy if { suspense: true } is used
        * React 18 will always resolve the Suspense boundary on the server-side, effectively ignoring the ssr option
@@ -122,19 +126,20 @@ export default function dynamic<P = {}>(
        * React.lazy that can suspense on the server-side while only loading the component on the client-side
        */
       if (loadableOptions.ssr === false) {
-        loadableOptions.ssr = true
         console.warn(
           `"ssr: false" is ignored by next/dynamic because you can not enable "suspense" while disabling "ssr" at the same time. Read more: https://nextjs.org/docs/messages/invalid-dynamic-suspense`
         )
       }
 
       if (loadableOptions.loading != null) {
-        loadableOptions.loading = undefined
         console.warn(
           `"loading" is ignored by next/dynamic because you have enabled "suspense". Place your loading element in your suspense boundary's "fallback" prop instead. Read more: https://nextjs.org/docs/messages/invalid-dynamic-suspense`
         )
       }
     }
+
+    delete loadableOptions.ssr
+    delete loadableOptions.loading
   }
 
   // coming from build/babel/plugins/react-loadable-plugin.js

test/integration/next-dynamic/test/index.test.js

@@ -31,6 +31,12 @@ function runTests() {
     // Failure case is 'Index<!-- -->3<!-- --><!-- -->'
     expect(text).toBe('Index<!-- -->1<!-- -->2<!-- -->3<!-- -->4<!-- -->4')
     expect(await browser.eval('window.caughtErrors')).toBe('')
+
+    // should not print "invalid-dynamic-suspense" warning in browser's console
+    const logs = (await browser.log()).map((log) => log.message).join('\n')
+    expect(logs).not.toContain(
+      'https://nextjs.org/docs/messages/invalid-dynamic-suspense'
+    )
   })
 }