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.

Sign up for GitHub

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

Jump to bottom

MSW Turbopack fails to import #66058

Closed
joaopedrodcf opened this issue May 21, 2024 · 7 comments
Closed

MSW Turbopack fails to import #66058

joaopedrodcf opened this issue May 21, 2024 · 7 comments
Labels
bug Issue was opened via the bug report template. linear: turbopack Confirmed issue that is tracked by the Turbopack team. locked Turbopack Related to Turbopack with Next.js.

Comments

@joaopedrodcf
Copy link

Link to the code that reproduces this issue

https://github.com/joaopedrodcf/nextjs-bug-msw

To Reproduce

  1. Yarn
  2. Yarn dev --turbo
  3. Open localhost:3000 and an error will appear
image

Current vs. Expected behavior

yarn dev works correctly but yarn dev --turbo doesn't work

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 23.4.0: Fri Mar 15 00:10:42 PDT 2024; root:xnu-10063.101.17~1/RELEASE_ARM64_T6000
  Available memory (MB): 16384
  Available CPU cores: 8
Binaries:
  Node: 20.9.0
  npm: 10.1.0
  Yarn: 1.22.22
  pnpm: 9.1.1
Relevant Packages:
  next: 14.2.3 // Latest available version is detected (14.2.3).
  eslint-config-next: 14.2.3
  react: 18.3.1
  react-dom: 18.3.1
  typescript: 5.4.5
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

Turbopack

Which stage(s) are affected? (Select all that apply)

next dev (local)

Additional context

This is the least complex setup I could create to showcase that when using turbo it breaks, I also tried to install the latest canary and the problem still persisted, 14.3.0-canary.75

I thought I remember that back in the day I could run with turbo and with msw but I tried a bunch of older versions and it didn't work.
@joaopedrodcf joaopedrodcf added the bug Issue was opened via the bug report template. label May 21, 2024
@github-actions github-actions bot added the Turbopack Related to Turbopack with Next.js. label May 21, 2024
@timneutkens timneutkens added the linear: turbopack Confirmed issue that is tracked by the Turbopack team. label May 23, 2024
timneutkens added a commit that referenced this issue May 23, 2024
@timneutkens
Add failing test for #66058
b735c3b
@timneutkens timneutkens mentioned this issue May 23, 2024
Merged
@timneutkens
Copy link
Member

Thanks for the clear reproduction! Did some digging and created an even more minimal reproduction + test in #66128 👍 We'll take a look into fixing this.

@joaopedrodcf
Copy link
Author

@timneutkens thank you so much for the reply, and for creating a super simple reproduction step + test 🙏

@timneutkens timneutkens mentioned this issue May 24, 2024
Merged
@timneutkens
Copy link
Member

I've added the typeof window inlining feature to Turbopack here: vercel/turborepo#8211 and updated the PR shared above to implement it: #66128

Checking locally it passes the tests with those changes 🙌

@joaopedrodcf
Copy link
Author

Amazing 💪💪

timneutkens added a commit to vercel/turborepo that referenced this issue May 25, 2024
@timneutkens @kdy1
Add option for inlining typeof window (#8211)
9004d1b 
### Description

In Next.js with webpack we have a feature to inline `typeof window` in
server/client environments. That allows e.g.

```
"use client"
if(typeof window === 'undefined') {
  import('server-only-package')
}
```

Today with Turbopack above code causes `server-only-package` to still be
loaded in the browser compilation. Which causes issues like
vercel/next.js#66058 where the `if`/`else`
condition is used to use different modules for server and client.

This change is required to pass the test in
vercel/next.js#66058

Pushing up the Next.js changes in that PR.

<!--
  ✍️ Write a short summary of your work.
  If necessary, include relevant screenshots.
-->

### Testing Instructions

<!--
  Give a quick description of steps to test your changes.
-->

---------

Co-authored-by: Donny/강동윤 <kdy1997.dev@gmail.com>
timneutkens added a commit that referenced this issue May 25, 2024
@timneutkens
Add failing test for #66058
329f366
timneutkens added a commit that referenced this issue May 26, 2024
@timneutkens
Implement typeof window inlining for Turbopack (#66128)
216ce33 
## What?

Update: Implemented a feature in Turbopack for `typeof window` inlining:
vercel/turborepo#8211. Verified the changes in that
PR + this PR fix the test 💯


Turbopack upgrade:

* vercel/turborepo#8081 <!-- Alexander Lyon - add
support for upload speed / remaining in the cache upload step -->
* vercel/turborepo#7673 <!-- Alexander Lyon -
[turbo-trace] add additional filter modes for allocs over time -->
* vercel/turborepo#8191 <!-- Will Binns-Smith -
Turbopack: Register react refresh exports in module factory -->
* vercel/turborepo#8195 <!-- Donny/강동윤 - feat:
Remove lint for `grid-template-areas`" -->
* vercel/turborepo#8207 <!-- Benjamin Woodruff -
chore: Remove unused `base16` dependency and dead `hex` wrapper function
-->
* vercel/turborepo#8185 <!-- Tim Neutkens - Update
contributing guide to mention snapshot test updating -->
* vercel/turborepo#8211 <!-- Tim Neutkens - Add
option for inlining typeof window -->
* vercel/turborepo#8214 <!-- Will Binns-Smith -
Revert "Turbopack: Register react refresh exports in module factory
(#8191)" -->

---

Previous PR description::

Implements a failing test for issue #66058.
The test only fails when using Turbopack.

While digging into that issue I found that `typeof window` replacement
is either not applied or does not cause `import()` or `require()` to be
removed. In the reproduction that approach is used to load a particular
package only in Node.js and another only in the browser.

<!-- Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that
you follow the checklist sections below.
Choose the right checklist for the change(s) that you're making:

## For Contributors

### Improving Documentation

- Run `pnpm prettier-fix` to fix formatting issues before opening the
PR.
- Read the Docs Contribution Guide to ensure your contribution follows
the docs guidelines:
https://nextjs.org/docs/community/contribution-guide

### Adding or Updating Examples

- The "examples guidelines" are followed from our contributing doc
https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md
- Make sure the linting passes by running `pnpm build && pnpm lint`. See
https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md

### Fixing a bug

- Related issues linked using `fixes #number`
- Tests added. See:
https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md

### Adding a feature

- Implements an existing feature request or RFC. Make sure the feature
request has been accepted for implementation before opening a PR. (A
discussion must be opened, see
https://github.com/vercel/next.js/discussions/new?category=ideas)
- Related issues/discussions are linked using `fixes #number`
- e2e tests added
(https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs)
- Documentation added
- Telemetry added. In case of a feature if it's used or not.
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md


## For Maintainers

- Minimal description (aim for explaining to someone not on the team to
understand the PR)
- When linking to a Slack thread, you might want to share details of the
conclusion
- Link both the Linear (Fixes NEXT-xxx) and the GitHub issues
- Add review comments if necessary to explain to the reviewer the logic
behind a change

### What?

### Why?

### How?

Closes NEXT-
Fixes #

-->
@timneutkens
Copy link
Member

Triggered a new canary with the fix, will be available in 15-30mins.

@joaopedrodcf
Copy link
Author

Worked perfectly 🙏

@github-actions GitHub Actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for 2 weeks. 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 Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Issue was opened via the bug report template. linear: turbopack Confirmed issue that is tracked by the Turbopack team. locked Turbopack Related to Turbopack with Next.js.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@timneutkens @joaopedrodcf