Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

appDir + async server component + css-modules causes syntax error #42466

Closed
1 task done
mxmul opened this issue Nov 4, 2022 · 7 comments · Fixed by #43288
Closed
1 task done

appDir + async server component + css-modules causes syntax error #42466

mxmul opened this issue Nov 4, 2022 · 7 comments · Fixed by #43288
Assignees
Labels
area: app App directory (appDir: true)

Comments

@mxmul
Copy link

mxmul commented Nov 4, 2022

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 22.1.0: Sun Oct  9 20:14:30 PDT 2022; root:xnu-8792.41.9~2/RELEASE_ARM64_T8103
Binaries:
  Node: 18.11.0
  npm: 8.19.2
  Yarn: 1.22.10
  pnpm: N/A
Relevant packages:
  next: 13.0.2
  eslint-config-next: N/A
  react: 18.2.0
  react-dom: 18.2.0

What browser are you using? (if relevant)

Chrome, Safari

How are you deploying your application? (if relevant)

next dev

Describe the Bug

When using the experimental /app mode with the following setup, RSC seems to stream in a malformed script tag, causing a crash:

  • Uses /app directory
  • Root page has both a layout.tsx and a loading.tsx
  • page.tsx exports an async server component
  • page.tsx uses a class name from a CSS module, styles.module.css

Screenshot 2022-11-03 at 10 14 23 PM

Screenshot 2022-11-03 at 10 11 36 PM

Unexpected string literal ",". Parse error.

See minimal repro in code here: mxmul/next13-async-component-css-modules-repro@c562d4d

Expected Behavior

Page should render the loading state immediately, then the loaded content.

e.g.
Screenshot 2022-11-03 at 10 14 44 PM

Link to reproduction

https://github.com/mxmul/next13-async-component-css-modules-repro

To Reproduce

Clone the repo, and run yarn dev.

The first time you load the page, it seems to render properly. You see a loading state, then the rendered page:

Screenshot 2022-11-03 at 10 14 44 PM

After refreshing the page, you should see next crash with a syntax error:

Screenshot 2022-11-03 at 10 14 23 PM

@mxmul mxmul added the bug Issue was opened via the bug report template. label Nov 4, 2022
@balazsorban44
Copy link
Member

balazsorban44 commented Nov 6, 2022

I do not seem to be able to reproduce this. Can you try next@canary? Can you see any more information in the terminal?

After refreshing the page

Is this a hard refresh (eg. F5) or hot reload (eg. editing and saving a file, in that case which one?)

@balazsorban44 balazsorban44 added type: needs investigation area: app App directory (appDir: true) labels Nov 6, 2022
@mxmul
Copy link
Author

mxmul commented Nov 6, 2022

It still reproduces with next@canary. No errors or warnings in the terminal. By "refresh", I mean a Cmd+R.

Something I just discovered is that it reproduces in Node.js 18 but not Node.js 17

@mxmul

This comment was marked as outdated.

@chrisco512
Copy link

Getting this same error Node v18.7.0, next@13.0.1.

@feedthejim
Copy link
Contributor

feedthejim commented Nov 7, 2022

Ok I investigated a bit, so I can confirm that this happens with Node 18 and not Node 16.

It does not reproduce on the first hit to the page but on all of them afterwards (refresh or not). What causes the error is that on subsequent page loads, we seem to miss a part of the script payload.

what we are suppose to inject

    <script>
      $RC = function (b, c, e) {
        c = document.getElementById(c)
        c.parentNode.removeChild(c)
        var a = document.getElementById(b)
        if (a) {
          b = a.previousSibling
          if (e) (b.data = '$!'), a.setAttribute('data-dgst', e)
          else {
            e = b.parentNode
            a = b.nextSibling
            var f = 0
            do {
              if (a && 8 === a.nodeType) {
                var d = a.data
                if ('/$' === d)
                  if (0 === f) break
                  else f--
                else ('$' !== d && '$?' !== d && '$!' !== d) || f++
              }
              d = a.nextSibling
              e.removeChild(a)
              a = d
            } while (a)
            for (; c.firstChild; ) e.insertBefore(c.firstChild, a)
            b.data = '$'
          }
          b._reactRetry && b._reactRetry()
        }
      }
      $RM = new Map()
      $RR = function (p, q, v) {
        function r(l) {
          this.s = l
        }
        for (
          var t = $RC,
            u = $RM,
            m = new Map(),
            n = document,
            g,
            e,
            f = n.querySelectorAll(
              'link[data-precedence],style[data-precedence]'
            ),
            d = 0;
          (e = f[d++]);

        )
          m.set(e.dataset.precedence, (g = e))
        e = 0
        f = []
        for (var c, h, b, a; (c = v[e++]); ) {
          var k = 0
          h = c[k++]
          if ((b = u.get(h))) 'l' !== b.s && f.push(b)
          else {
            a = n.createElement('link')
            a.href = h
            a.rel = 'stylesheet'
            for (a.dataset.precedence = d = c[k++]; (b = c[k++]); )
              a.setAttribute(b, c[k++])
            b = a._p = new Promise(function (l, w) {
              a.onload = l
              a.onerror = w
            })
            b.then(r.bind(b, 'l'), r.bind(b, 'e'))
            u.set(h, b)
            f.push(b)
            c = m.get(d) || g
            c === g && (g = a)
            m.set(d, a)
            c
              ? c.parentNode.insertBefore(a, c.nextSibling)
              : ((d = n.head), d.insertBefore(a, d.firstChild))
          }
        }
        Promise.all(f).then(
          t.bind(null, p, q, ''),
          t.bind(null, p, q, 'Resource failed to load')
        )
      }
      $RR('B:0', 'S:0', [
        [
          '/_next/static/css/app_styles_module_css.css?ts=1667831282087',
          'high',
        ],
      ])
    </script>

what we end up receiving, hence the unexpected string

 <script>
    B:0","S:0",[["/_next/static/css/app_styles_module_css.css?ts=1667830880867","high"]])
  </script>

we seem to be missing some part of the payload🤔

Will investigate further.

var completeBoundaryWithStylesScript1FullBoth = stringToPrecomputedChunk(completeBoundary + ';' + completeBoundaryWithStyles + ';$RR("');

this particular buffer seems to get cleared out at some point, which causes the error

@feedthejim
Copy link
Contributor

feedthejim commented Nov 7, 2022

Found the bug, let's hope I can get facebook/react#25645 merged fast!

@timneutkens timneutkens added kind: bug and removed bug Issue was opened via the bug report template. labels Nov 8, 2022
sebmarkbage added a commit to facebook/react that referenced this issue Nov 22, 2022
## Edit

Went for another approach after talking with @gnoff. The approach is
now:
- add a dev-only error when a precomputed chunk is too big to be written
- suggest to copy it before passing it to `writeChunk`

This PR also includes porting the React Float tests to use the browser
build of Fizz so that we can test it out on that environment (which is
the one used by next).

<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn debug-test --watch TestName`, open
`chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

Someone reported [a bug](vercel/next.js#42466)
in Next.js that pointed to an issue with Node 18 in the streaming
renderer when using importing a CSS module where it only returned a
malformed bootstraping script only after loading the page once.

After investigating a bit, here's what I found:

- when using a CSS module in Next, we go into this code path, which
writes the aforementioned bootstrapping script


https://github.com/facebook/react/blob/5f7ef8c4cbe824ef126a947b7ae0e1c07b143357/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js#L2443-L2447

- the reason for the malformed script is that
`completeBoundaryWithStylesScript1FullBoth` is emptied after the call to
`writeChunk`
- it gets emptied in `writeChunk` because we stream the chunk directly
without copying it in this codepath

https://github.com/facebook/react/blob/a438590144d2ad40865b58e0c0e69595fc1aa377/packages/react-server/src/ReactServerStreamConfigBrowser.js#L63
- the reason why it only happens from Node 18 is because the Webstreams
APIs are available natively from that version and in their
implementation, [`enqueue` transfers the array buffer
ownership](https://github.com/nodejs/node/blob/9454ba6138d11e8a4d18b073de25781cad4bd2c8/lib/internal/webstreams/readablestream.js#L2641),
thus making it unavailable/empty for subsequent calls. In older Node
versions, we don't encounter the bug because we are using a polyfill in
Next.js, [which does not implement properly the array buffer transfer
behaviour](https://cs.github.com/MattiasBuelens/web-streams-polyfill/blob/d354a7457ca8a24030dbd0a135ee40baed7c774d/src/lib/abstract-ops/ecmascript.ts#L16).

I think the proper fix for this is to clone the array buffer before
enqueuing it. (we do this in the other code paths in the function later
on, see ```((currentView: any): Uint8Array).set(bytesToWrite,
writtenBytes);```





## How did you test this change?

Manually tested by applying the change in the compiled Next.js version.

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->

Co-authored-by: Sebastian Markbage <sebastian@calyptus.eu>
@huozhi huozhi mentioned this issue Nov 23, 2022
3 tasks
@kodiakhq kodiakhq bot closed this as completed in #43288 Nov 23, 2022
kodiakhq bot pushed a commit that referenced this issue Nov 23, 2022
Update pre-compiled react to fix the streaming issue with node 18.
x-ref: facebook/react#25645

Fixes: #42466
Manually tested locally works for the reproduction from the issue

## Bug

- [x] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Errors have a helpful link attached, see [`contributing.md`](https://github.com/vercel/next.js/blob/canary/contributing.md)
tiziodcaio pushed a commit to tiziodcaio/react that referenced this issue Nov 26, 2022
…k#25645)

## Edit

Went for another approach after talking with @gnoff. The approach is
now:
- add a dev-only error when a precomputed chunk is too big to be written
- suggest to copy it before passing it to `writeChunk`

This PR also includes porting the React Float tests to use the browser
build of Fizz so that we can test it out on that environment (which is
the one used by next).

<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn debug-test --watch TestName`, open
`chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

Someone reported [a bug](vercel/next.js#42466)
in Next.js that pointed to an issue with Node 18 in the streaming
renderer when using importing a CSS module where it only returned a
malformed bootstraping script only after loading the page once.

After investigating a bit, here's what I found:

- when using a CSS module in Next, we go into this code path, which
writes the aforementioned bootstrapping script


https://github.com/facebook/react/blob/5f7ef8c4cbe824ef126a947b7ae0e1c07b143357/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js#L2443-L2447

- the reason for the malformed script is that
`completeBoundaryWithStylesScript1FullBoth` is emptied after the call to
`writeChunk`
- it gets emptied in `writeChunk` because we stream the chunk directly
without copying it in this codepath

https://github.com/facebook/react/blob/a438590144d2ad40865b58e0c0e69595fc1aa377/packages/react-server/src/ReactServerStreamConfigBrowser.js#L63
- the reason why it only happens from Node 18 is because the Webstreams
APIs are available natively from that version and in their
implementation, [`enqueue` transfers the array buffer
ownership](https://github.com/nodejs/node/blob/9454ba6138d11e8a4d18b073de25781cad4bd2c8/lib/internal/webstreams/readablestream.js#L2641),
thus making it unavailable/empty for subsequent calls. In older Node
versions, we don't encounter the bug because we are using a polyfill in
Next.js, [which does not implement properly the array buffer transfer
behaviour](https://cs.github.com/MattiasBuelens/web-streams-polyfill/blob/d354a7457ca8a24030dbd0a135ee40baed7c774d/src/lib/abstract-ops/ecmascript.ts#L16).

I think the proper fix for this is to clone the array buffer before
enqueuing it. (we do this in the other code paths in the function later
on, see ```((currentView: any): Uint8Array).set(bytesToWrite,
writtenBytes);```





## How did you test this change?

Manually tested by applying the change in the compiled Next.js version.

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->

Co-authored-by: Sebastian Markbage <sebastian@calyptus.eu>
mofeiZ pushed a commit to mofeiZ/react that referenced this issue Dec 2, 2022
…k#25645)

## Edit

Went for another approach after talking with @gnoff. The approach is
now:
- add a dev-only error when a precomputed chunk is too big to be written
- suggest to copy it before passing it to `writeChunk`

This PR also includes porting the React Float tests to use the browser
build of Fizz so that we can test it out on that environment (which is
the one used by next).

<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn debug-test --watch TestName`, open
`chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

Someone reported [a bug](vercel/next.js#42466)
in Next.js that pointed to an issue with Node 18 in the streaming
renderer when using importing a CSS module where it only returned a
malformed bootstraping script only after loading the page once.

After investigating a bit, here's what I found:

- when using a CSS module in Next, we go into this code path, which
writes the aforementioned bootstrapping script


https://github.com/facebook/react/blob/5f7ef8c4cbe824ef126a947b7ae0e1c07b143357/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js#L2443-L2447

- the reason for the malformed script is that
`completeBoundaryWithStylesScript1FullBoth` is emptied after the call to
`writeChunk`
- it gets emptied in `writeChunk` because we stream the chunk directly
without copying it in this codepath

https://github.com/facebook/react/blob/a438590144d2ad40865b58e0c0e69595fc1aa377/packages/react-server/src/ReactServerStreamConfigBrowser.js#L63
- the reason why it only happens from Node 18 is because the Webstreams
APIs are available natively from that version and in their
implementation, [`enqueue` transfers the array buffer
ownership](https://github.com/nodejs/node/blob/9454ba6138d11e8a4d18b073de25781cad4bd2c8/lib/internal/webstreams/readablestream.js#L2641),
thus making it unavailable/empty for subsequent calls. In older Node
versions, we don't encounter the bug because we are using a polyfill in
Next.js, [which does not implement properly the array buffer transfer
behaviour](https://cs.github.com/MattiasBuelens/web-streams-polyfill/blob/d354a7457ca8a24030dbd0a135ee40baed7c774d/src/lib/abstract-ops/ecmascript.ts#L16).

I think the proper fix for this is to clone the array buffer before
enqueuing it. (we do this in the other code paths in the function later
on, see ```((currentView: any): Uint8Array).set(bytesToWrite,
writtenBytes);```





## How did you test this change?

Manually tested by applying the change in the compiled Next.js version.

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->

Co-authored-by: Sebastian Markbage <sebastian@calyptus.eu>
mofeiZ pushed a commit to mofeiZ/react that referenced this issue Dec 5, 2022
…k#25645)

## Edit

Went for another approach after talking with @gnoff. The approach is
now:
- add a dev-only error when a precomputed chunk is too big to be written
- suggest to copy it before passing it to `writeChunk`

This PR also includes porting the React Float tests to use the browser
build of Fizz so that we can test it out on that environment (which is
the one used by next).

<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn debug-test --watch TestName`, open
`chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

Someone reported [a bug](vercel/next.js#42466)
in Next.js that pointed to an issue with Node 18 in the streaming
renderer when using importing a CSS module where it only returned a
malformed bootstraping script only after loading the page once.

After investigating a bit, here's what I found:

- when using a CSS module in Next, we go into this code path, which
writes the aforementioned bootstrapping script


https://github.com/facebook/react/blob/5f7ef8c4cbe824ef126a947b7ae0e1c07b143357/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js#L2443-L2447

- the reason for the malformed script is that
`completeBoundaryWithStylesScript1FullBoth` is emptied after the call to
`writeChunk`
- it gets emptied in `writeChunk` because we stream the chunk directly
without copying it in this codepath

https://github.com/facebook/react/blob/a438590144d2ad40865b58e0c0e69595fc1aa377/packages/react-server/src/ReactServerStreamConfigBrowser.js#L63
- the reason why it only happens from Node 18 is because the Webstreams
APIs are available natively from that version and in their
implementation, [`enqueue` transfers the array buffer
ownership](https://github.com/nodejs/node/blob/9454ba6138d11e8a4d18b073de25781cad4bd2c8/lib/internal/webstreams/readablestream.js#L2641),
thus making it unavailable/empty for subsequent calls. In older Node
versions, we don't encounter the bug because we are using a polyfill in
Next.js, [which does not implement properly the array buffer transfer
behaviour](https://cs.github.com/MattiasBuelens/web-streams-polyfill/blob/d354a7457ca8a24030dbd0a135ee40baed7c774d/src/lib/abstract-ops/ecmascript.ts#L16).

I think the proper fix for this is to clone the array buffer before
enqueuing it. (we do this in the other code paths in the function later
on, see ```((currentView: any): Uint8Array).set(bytesToWrite,
writtenBytes);```





## How did you test this change?

Manually tested by applying the change in the compiled Next.js version.

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->

Co-authored-by: Sebastian Markbage <sebastian@calyptus.eu>
@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: app App directory (appDir: true)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants